netdev - Re: [PATCH net-next 1/3] net: sched: cls_api: add skip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a4798b5d-1a8a-41ab-842f-52e8c7ac00ed@fiberby.net>
Date: Thu, 15 Feb 2024 23:34:02 +0000
From: Asbjørn Sloth Tønnesen <ast@...erby.net>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
 Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, llu@...erby.dk, Vlad Buslov
 <vladbu@...dia.com>, Marcelo Ricardo Leitner <mleitner@...hat.com>
Subject: Re: [PATCH net-next 1/3] net: sched: cls_api: add skip_sw counter

Hi Jamal,

Thank you for the review.

On 2/15/24 17:39, Jamal Hadi Salim wrote:
> +Cc Vlad and Marcelo..
> 
> On Thu, Feb 15, 2024 at 11:06 AM Asbjørn Sloth Tønnesen <ast@...erby.net> wrote:
>>
>> Maintain a count of skip_sw filters.
>>
>> This counter is protected by the cb_lock, and is updated
>> at the same time as offloadcnt.
>>
>> Signed-off-by: Asbjørn Sloth Tønnesen <ast@...erby.net>
>> ---
>>   include/net/sch_generic.h | 1 +
>>   net/sched/cls_api.c       | 4 ++++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
>> index 934fdb977551..46a63d1818a0 100644
>> --- a/include/net/sch_generic.h
>> +++ b/include/net/sch_generic.h
>> @@ -476,6 +476,7 @@ struct tcf_block {
>>          struct flow_block flow_block;
>>          struct list_head owner_list;
>>          bool keep_dst;
>> +       atomic_t skipswcnt; /* Number of skip_sw filters */
>>          atomic_t offloadcnt; /* Number of oddloaded filters */
> 
> For your use case is skipswcnt ever going to be any different than offloadcnt?

No, we only use skip_sw filters, since we only use TC as a control path to
install skip_sw rules into hardware.

AFAICT offloadcnt is the sum of skip_sw filters, and filters with no flags which
have implicitly been offloaded.

The reason that I didn't just use offloadcnt, is that I'm not sure if it is
acceptable to treat implicitly offloaded rules without skip_sw, as if they were
explicitly skip_sw. It sounds reasonable, given that the filters without skip_* flags
shouldn't really care.

I tried to only trigger the TC bypass, in the cases that I was absolutely sure would
be safe as a first step.


> 
> cheers,
> jamal
> 
>>          unsigned int nooffloaddevcnt; /* Number of devs unable to do offload */
>>          unsigned int lockeddevcnt; /* Number of devs that require rtnl lock. */
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index ca5676b2668e..397c3d29659c 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -3483,6 +3483,8 @@ static void tcf_block_offload_inc(struct tcf_block *block, u32 *flags)
>>          if (*flags & TCA_CLS_FLAGS_IN_HW)
>>                  return;
>>          *flags |= TCA_CLS_FLAGS_IN_HW;
>> +       if (tc_skip_sw(*flags))
>> +               atomic_inc(&block->skipswcnt);
>>          atomic_inc(&block->offloadcnt);
>>   }
>>
>> @@ -3491,6 +3493,8 @@ static void tcf_block_offload_dec(struct tcf_block *block, u32 *flags)
>>          if (!(*flags & TCA_CLS_FLAGS_IN_HW))
>>                  return;
>>          *flags &= ~TCA_CLS_FLAGS_IN_HW;
>> +       if (tc_skip_sw(*flags))
>> +               atomic_dec(&block->skipswcnt);
>>          atomic_dec(&block->offloadcnt);
>>   }
>>
>> --
>> 2.43.0
>>

-- 
Best regards
Asbjørn Sloth Tønnesen
Network Engineer
Fiberby - AS42541