linux-audit.redhat.com archive mirror
 help / color / mirror / Atom feed
From: Ankur Arora <ankur.a.arora@oracle.com>
To: Paul Moore <paul@paul-moore.com>
Cc: konrad.wilk@oracle.com, linux-kernel@vger.kernel.org,
	eparis@redhat.com, linux-audit@redhat.com,
	boris.ostrovsky@oracle.com
Subject: Re: [PATCH 2/3] audit: annotate branch direction for audit_in_mask()
Date: Thu, 06 Oct 2022 17:57:26 -0700	[thread overview]
Message-ID: <875ygwbgcp.fsf@oracle.com> (raw)
In-Reply-To: <CAHC9VhTQ8HR4ZF1VNCwuFabTUU+Y+ooeDuSwNTozuWMo4RbgSw@mail.gmail.com>


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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


  reply	other threads:[~2022-10-07  0:57 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-27 22:59 [PATCH 0/3] improve audit syscall-exit latency Ankur Arora
2022-09-27 22:59 ` [PATCH 1/3] audit: cache ctx->major in audit_filter_syscall() Ankur Arora
2022-09-28 22:03   ` Paul Moore
2022-09-29 20:20     ` Ankur Arora
2022-10-17 18:23     ` Paul Moore
2022-09-30 17:45   ` Steve Grubb
2022-09-30 18:22     ` Paul Moore
2022-10-07  0:55     ` Ankur Arora
2022-09-27 22:59 ` [PATCH 2/3] audit: annotate branch direction for audit_in_mask() Ankur Arora
2022-09-28 22:26   ` Paul Moore
2022-09-29 20:19     ` Ankur Arora
2022-09-30 18:48       ` Paul Moore
2022-10-07  0:57         ` Ankur Arora [this message]
2022-09-27 22:59 ` [PATCH 3/3] audit: unify audit_filter_{uring(), inode_name(), syscall()} Ankur Arora
2022-09-28 22:54   ` Paul Moore
2022-09-29 20:23     ` [PATCH 3/3] audit: unify audit_filter_{uring(),inode_name(),syscall()} Ankur Arora
2022-10-07  0:49   ` [PATCH v2] audit: unify audit_filter_{uring(), inode_name(), syscall()} Ankur Arora
2022-10-13 23:11     ` Paul Moore
2022-10-14 16:53       ` [PATCH v2] audit: unify audit_filter_{uring(),inode_name(),syscall()} Ankur Arora
2022-10-17 18:26       ` [PATCH v2] audit: unify audit_filter_{uring(), inode_name(), syscall()} Paul Moore

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875ygwbgcp.fsf@oracle.com \
    --to=ankur.a.arora@oracle.com \
    --cc=boris.ostrovsky@oracle.com \
    --cc=eparis@redhat.com \
    --cc=konrad.wilk@oracle.com \
    --cc=linux-audit@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paul@paul-moore.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).