Received-SPF: pass (google.com: domain of linux-kernel+bounces-67841-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <a4798b5d-1a8a-41ab-842f-52e8c7ac00ed@fiberby.net>
Date: Thu, 15 Feb 2024 23:34:02 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 1/3] net: sched: cls_api: add skip_sw counter
Content-Language: en-US
To: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>, Jiri Pirko <jiri@resnulli.us>,
 Daniel Borkmann <daniel@iogearbox.net>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, llu@fiberby.dk, Vlad Buslov
 <vladbu@nvidia.com>, Marcelo Ricardo Leitner <mleitner@redhat.com>
References: <20240215160458.1727237-1-ast@fiberby.net>
 <20240215160458.1727237-2-ast@fiberby.net>
 <CAM0EoMndBjwC8Otx6th_dM_aV_r80NeLEke9C8PwzGt1q3vAMA@mail.gmail.com>
From: =?UTF-8?Q?Asbj=C3=B8rn_Sloth_T=C3=B8nnesen?= <ast@fiberby.net>
In-Reply-To: <CAM0EoMndBjwC8Otx6th_dM_aV_r80NeLEke9C8PwzGt1q3vAMA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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@fiberby.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@fiberby.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