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;
References: <20220927225944.2254360-1-ankur.a.arora@oracle.com>
 <20220927225944.2254360-3-ankur.a.arora@oracle.com>
 <CAHC9VhRBuBiuf6917fpP7n+Sy5fFTpzzEqRXfMGPCbnNU4jswg@mail.gmail.com>
User-agent: mu4e 1.4.10; emacs 27.2
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()
In-reply-to: <CAHC9VhRBuBiuf6917fpP7n+Sy5fFTpzzEqRXfMGPCbnNU4jswg@mail.gmail.com>
Date:   Thu, 29 Sep 2022 13:19:57 -0700
Message-ID: <8735cagcgi.fsf@oracle.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?q6tSaMAzrwkXL1iGOB/rZjB0fbdeehEDtgAjzKtZkjD+DccKkH0YPOuay9e5?=
 =?us-ascii?Q?pOeNwsqfmwzKzMI2z9E7twt7iMfMjXheb7ayPtFedyPGmr3Lh9CUetVUzxOV?=
 =?us-ascii?Q?PqntNHxJVrH9pzOao9QxdzK+ehdMueSJV61vG4pFOaMrYWESowepaY6fvw76?=
 =?us-ascii?Q?Tcq73YrOSvJkcsPfEvo9qbQPGT2Pvrgs6tRyiKyL4Q3jEyi5rlYLCBD9CZ1g?=
 =?us-ascii?Q?pKio8Y/R5nkRcDg7NUoK97KDILH4ERgU4sDEEBQtQ0OsDhpZdjm1WS42SUeb?=
 =?us-ascii?Q?8nVdJFVaizYRtbuEbLYMbjVp0N/lpjr2iNB/Z4kMB9GCn7evn1ZQ2u3QCXgP?=
 =?us-ascii?Q?9hLJiNeDYB2htkHwMQItEOl0zwzKtwPFeBIvbbDW47YZyacyvKk9TbqiwMHa?=
 =?us-ascii?Q?N8Co2juVMQmm83IpJRLzTMIXLngnV1x3Rc2153iX6MDDksraW7LgDTd7gxuB?=
 =?us-ascii?Q?Am90rmuzc+jMEzHvsWtrQUCtb1V55J8F0V3+/rZEL0b2JSk/dbT9Rx9THHwJ?=
 =?us-ascii?Q?+2nBC4A2XlxUDPcEpPZt/+esOUxJleQuKscNe170VAZuwQdeMNp7f4dQTbJJ?=
 =?us-ascii?Q?6ZRTHYIffnopjY0Cw2DEBZ3FRSV+RLp/L/Z5bq6bSACa4wfYCybKaOCzIvqY?=
 =?us-ascii?Q?eXkwOA0trOca9Rj6Q/aQUr+zxLuZjibNQBjIn79hLMLlzT05H6gvHMIIlAbN?=
 =?us-ascii?Q?2Q8Qk0O7sOUGYSXnWmEVLw9CSY9uLBd+uh6lwGj03ikl0L+0+e+ryVokdta6?=
 =?us-ascii?Q?zWQ4u+EfNEiiIgOZPIt5DDAL2CeJGz2cnyXDmYU+jPsxg0IdIu1Mg7HUSHBP?=
 =?us-ascii?Q?DyyQQ/pQN+7EVTDry4DFtwFuPCWIAZLpr4SfkeeuheyH6JB2vESy4b5EqjZW?=
 =?us-ascii?Q?Xau2jnuIWsAL0DnVK79sN6J9JgKwplkynw+s/VzjwcqylloNpYzbr1kYc7W6?=
 =?us-ascii?Q?0ouEwaJzX8ReeQEkaGwM45nxLpGEg4BHt6vC5x3fOYbcdjH36YBjtEauI3HR?=
 =?us-ascii?Q?NN/eRtnomzyJXmNUARMtMB3QZPsb0g3CS2Hoj6eYbO05jFAWKC09NsJfPL3t?=
 =?us-ascii?Q?X7YlyObHPjnN0DmI75ib/Qfv8Yf0NZqltVGW1YapIQV10ba0gYQEXVMCJhXS?=
 =?us-ascii?Q?ETSHKJebjXeYVbrIUjEaY7t37Bn+yCgSwENXjBQlkMHm0wIg8c+gTaxxTOFu?=
 =?us-ascii?Q?BT+TFlQE2yIs0otcP6Wm+GpkOF/XDehcw2JZ7wKWeXUXxJ2i06nRNBzybekl?=
 =?us-ascii?Q?3jjrvDwXs96XMLrJMz3J5phK8SbRrRhlhDPdCeTGJQy1bwWtwzpLgVpQfDGo?=
 =?us-ascii?Q?S5xyw/sB2tlWEKS4cpQAZc7DOre484kNaU+t2gFvS25ee6erS7WmanJCd2/2?=
 =?us-ascii?Q?OGZZ4BZY3zTapSauOWsvvvX05PL6FNZ8dqLKXCYlASnu+1SoEl0Xy7tUOIaq?=
 =?us-ascii?Q?lVKdtp6LSBtfbTtats1q8LobFEtOohsnQl/6YNGJ+evEIoBlzQu5eMG3iKIt?=
 =?us-ascii?Q?j0uvxHa9WMeE3CD4jgP+HtkGzvDPDlgqqAwIL81IRfqFqiIFpwq5pr9ZTGRT?=
 =?us-ascii?Q?eNWEG6ME300YoS1FEKjNySMY+TpB4bpkEvdB2NMetMnQ20FKYBokfFJLNNbb?=
 =?us-ascii?Q?2Q=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: effd1dd9-fd0f-40c5-d1ec-08daa257fad3
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2022 20:19:58.7261
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: CtCRR1CmsSlCHeFhQ/MK7QQ+ciW2FgL07Vz1bEcoZhe+QN7dJIdPk+5eDm1G8ywkMGefRtqKJc4XDzThZq0n8VT17dhc8iXNslPHSQndICM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB5779
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-29_11,2022-09-29_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0 phishscore=0
 mlxscore=0 mlxlogscore=999 adultscore=0 spamscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000
 definitions=main-2209290128
X-Proofpoint-ORIG-GUID: leFE74JVTL-7_7BHeCE_T-sRm69-P-FR
X-Proofpoint-GUID: leFE74JVTL-7_7BHeCE_T-sRm69-P-FR
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Paul Moore <paul@paul-moore.com> writes:

> On Tue, Sep 27, 2022 at 7:00 PM Ankur Arora <ankur.a.arora@oracle.com> wrote:
>>
>> With sane audit rules, audit logging would only be triggered
>> infrequently. Keeping this in mind, annotate audit_in_mask() as
>> unlikely() to allow the compiler to pessimize the call to
>> audit_filter_rules().
>>
>> This allows GCC to invert the branch direction for the audit_filter_rules()
>> basic block in this loop:
>>
>>         list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_EXIT], list) {
>>                 if (audit_in_mask(&e->rule, major) &&
>>                     audit_filter_rules(tsk, &e->rule, ctx, NULL,
>>                                        &state, false)) {
>>                         ...
>>
>> such that it executes the common case in a straight line fashion.
>>
>> On a Skylakex system change in getpid() latency (all results
>> aggregated across 12 boot cycles):
>>
>>          Min     Mean    Median   Max      pstdev
>>          (ns)    (ns)    (ns)     (ns)
>>
>>  -    196.63   207.86  206.60  230.98      (+- 3.92%)
>>  +    173.11   182.51  179.65  202.09      (+- 4.34%)
>>
>> Performance counter stats for 'bin/getpid' (3 runs) go from:
>>     cycles               805.58  (  +-  4.11% )
>>     instructions        1654.11  (  +-   .05% )
>>     IPC                    2.06  (  +-  3.39% )
>>     branches             430.02  (  +-   .05% )
>>     branch-misses          1.55  (  +-  7.09% )
>>     L1-dcache-loads      440.01  (  +-   .09% )
>>     L1-dcache-load-misses  9.05  (  +- 74.03% )
>>
>>  to:
>>     cycles               706.13  (  +-  4.13% )
>>     instructions        1654.70  (  +-   .06% )
>>     IPC                    2.35  (  +-  4.25% )
>>     branches             430.99  (  +-   .06% )
>>     branch-misses          0.50  (  +-  2.00% )
>>     L1-dcache-loads      440.02  (  +-   .07% )
>>     L1-dcache-load-misses  5.22  (  +- 82.75% )
>>
>> (Both aggregated over 12 boot cycles.)
>>
>> cycles: performance improves on average by ~100 cycles/call. IPC
>> improves commensurately. Two reasons for this improvement:
>>
>>   * one fewer branch mispred: no obvious reason for this
>>     branch-miss reduction. There is no significant change in
>>     basic-block structure (apart from the branch inversion.)
>>
>>   * the direction of the branch for the call is now inverted, so it
>>     chooses the not-taken direction more often. The issue-latency
>>     for not-taken branches is often cheaper.
>>
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> ---
>>  kernel/auditsc.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> 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.

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.

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?


Thanks
Ankur

> While I appreciate the work you are doing to improve
> audit performance, I don't think this is something I want to merge,
> I'm sorry.


>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 533b087c3c02..bf26f47b5226 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -789,7 +789,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
>>         return AUDIT_STATE_BUILD;
>>  }
>>
>> -static int audit_in_mask(const struct audit_krule *rule, unsigned long val)
>> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
>>  {
>>         int word, bit;
>>
>> @@ -850,12 +850,13 @@ static void audit_filter_syscall(struct task_struct *tsk,
>>
>>         rcu_read_lock();
>>         list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_EXIT], list) {
>> -               if (audit_in_mask(&e->rule, major) &&
>> -                   audit_filter_rules(tsk, &e->rule, ctx, NULL,
>> -                                      &state, false)) {
>> -                       rcu_read_unlock();
>> -                       ctx->current_state = state;
>> -                       return;
>> +               if (unlikely(audit_in_mask(&e->rule, major))) {
>> +                       if (audit_filter_rules(tsk, &e->rule, ctx, NULL,
>> +                                              &state, false)) {
>> +                               rcu_read_unlock();
>> +                               ctx->current_state = state;
>> +                               return;
>> +                       }
>>                 }
>>         }
>>         rcu_read_unlock();
>> --
>> 2.31.1


--
ankur