Messages in this thread | | | From | Ankur Arora <> | Subject | Re: [PATCH 2/3] audit: annotate branch direction for audit_in_mask() | Date | Thu, 06 Oct 2022 17:57:26 -0700 |
| |
Paul Moore <paul@paul-moore.com> writes:
> On Thu, Sep 29, 2022 at 4:20 PM Ankur Arora <ankur.a.arora@oracle.com> wrote: >> Paul Moore <paul@paul-moore.com> writes: >> > I generally dislike merging likely()/unlikely() additions to code >> > paths that can have varying levels of performance depending on runtime >> > configuration. >> >> I think that's fair, and in this particular case the benchmark is quite >> contrived. >> >> But, just to elaborate a bit more on why that unlikely() clause made >> sense to me: it seems to me that audit typically would be triggered for >> control syscalls and the ratio between control and non-control ones >> would be fairly lopsided. > > I understand, and there is definitely some precedence in the audit > code for using likely()/unlikely() in a manner similar as you > described, but I'll refer to my previous comments - it's not something > I like. As a general rule, aside from the unlikely() calls in the > audit wrappers present in include/linux/audit.h I would suggest not > adding any new likely()/unlikely() calls. > >> Let me see if I can rewrite the conditional in a different way to get a >> similar effect but I suspect that might be even more compiler dependent. > > I am okay with ordering conditionals to make the common case the > short-circuit case.
So I played around with a bunch of different combinations of the conditionals but nothing really improved the code all that much.
Just sent out v2 dropping the unlikely() clause.
Thanks Ankur
> >> Also, let me run the audit-testsuite this time. Is there a good test >> there that you would recommend that might serve as a more representative >> workload? > > Probably not. The audit-testsuite is intended to be a quick, easy to > run regression test that can be used by developers to help reduce > audit breakage. It is not representative of any particular workload, > and is definitely not comprehensive (it is woefully lacking in several > areas unfortunately).
-- ankur
| |