All of lore.kernel.org
 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


WARNING: multiple messages have this Message-ID (diff)
From: Ankur Arora <ankur.a.arora@oracle.com>
To: Paul Moore <paul@paul-moore.com>
Cc: Ankur Arora <ankur.a.arora@oracle.com>,
	linux-audit@redhat.com, eparis@redhat.com,
	linux-kernel@vger.kernel.org, boris.ostrovsky@oracle.com,
	konrad.wilk@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

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

Thread overview: 40+ 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 ` Ankur Arora
2022-09-27 22:59 ` [PATCH 1/3] audit: cache ctx->major in audit_filter_syscall() Ankur Arora
2022-09-27 22:59   ` Ankur Arora
2022-09-28 22:03   ` Paul Moore
2022-09-28 22:03     ` Paul Moore
2022-09-29 20:20     ` Ankur Arora
2022-09-29 20:20       ` Ankur Arora
2022-10-17 18:23     ` Paul Moore
2022-10-17 18:23       ` Paul Moore
2022-09-30 17:45   ` Steve Grubb
2022-09-30 17:45     ` Steve Grubb
2022-09-30 18:22     ` Paul Moore
2022-09-30 18:22       ` Paul Moore
2022-10-07  0:55     ` Ankur Arora
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-27 22:59   ` Ankur Arora
2022-09-28 22:26   ` Paul Moore
2022-09-28 22:26     ` Paul Moore
2022-09-29 20:19     ` Ankur Arora
2022-09-29 20:19       ` Ankur Arora
2022-09-30 18:48       ` Paul Moore
2022-09-30 18:48         ` Paul Moore
2022-10-07  0:57         ` Ankur Arora [this message]
2022-10-07  0:57           ` Ankur Arora
2022-09-27 22:59 ` [PATCH 3/3] audit: unify audit_filter_{uring(), inode_name(), syscall()} Ankur Arora
2022-09-27 22:59   ` [PATCH 3/3] audit: unify audit_filter_{uring(),inode_name(),syscall()} Ankur Arora
2022-09-28 22:54   ` [PATCH 3/3] audit: unify audit_filter_{uring(), inode_name(), syscall()} Paul Moore
2022-09-28 22:54     ` [PATCH 3/3] audit: unify audit_filter_{uring(),inode_name(),syscall()} Paul Moore
2022-09-29 20:23     ` Ankur Arora
2022-09-29 20:23       ` Ankur Arora
2022-10-07  0:49   ` [PATCH v2] 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     ` [PATCH v2] audit: unify audit_filter_{uring(), inode_name(), syscall()} Paul Moore
2022-10-13 23:11       ` [PATCH v2] audit: unify audit_filter_{uring(),inode_name(),syscall()} Paul Moore
2022-10-14 16:53       ` Ankur Arora
2022-10-14 16:53         ` Ankur Arora
2022-10-17 18:26       ` [PATCH v2] audit: unify audit_filter_{uring(), inode_name(), syscall()} Paul Moore
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.