Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220927225944.2254360-1-ankur.a.arora@oracle.com>
 <20220927225944.2254360-3-ankur.a.arora@oracle.com> <CAHC9VhRBuBiuf6917fpP7n+Sy5fFTpzzEqRXfMGPCbnNU4jswg@mail.gmail.com>
 <8735cagcgi.fsf@oracle.com>
In-Reply-To: <8735cagcgi.fsf@oracle.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Fri, 30 Sep 2022 14:48:04 -0400
Message-ID: <CAHC9VhTQ8HR4ZF1VNCwuFabTUU+Y+ooeDuSwNTozuWMo4RbgSw@mail.gmail.com>
Subject: Re: [PATCH 2/3] audit: annotate branch direction for audit_in_mask()
To:     Ankur Arora <ankur.a.arora@oracle.com>
Cc:     linux-audit@redhat.com, eparis@redhat.com,
        linux-kernel@vger.kernel.org, boris.ostrovsky@oracle.com,
        konrad.wilk@oracle.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

> 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).

-- 
paul-moore.com