Re: [PATCH net-next 3/3] net: create and use NAPI version of tc_skb_ext_alloc()

[Jiri Pirko <jiri@resnulli.us>]

Wed, Feb 15, 2023 at 05:50:55PM CET, jhs@mojatatu.com wrote:
>On Tue, Feb 14, 2023 at 10:44 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> Try to use the cached skb_ext in the drivers.
>>
>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>> ---
>> CC: saeedm@nvidia.com
>> CC: leon@kernel.org
>> CC: jhs@mojatatu.com
>> CC: xiyou.wangcong@gmail.com
>> CC: jiri@resnulli.us
>> CC: roid@nvidia.com
>> CC: ozsh@nvidia.com
>> CC: paulb@nvidia.com
>> ---
>>  drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c | 2 +-
>>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c     | 2 +-
>>  include/net/pkt_cls.h                               | 9 +++++++++
>>  3 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
>> index 3b590cfe33b8..ffbed5a92eab 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c
>> @@ -770,7 +770,7 @@ static bool mlx5e_restore_skb_chain(struct sk_buff *skb, u32 chain, u32 reg_c1,
>>                 struct mlx5_eswitch *esw;
>>                 u32 zone_restore_id;
>>
>> -               tc_skb_ext = tc_skb_ext_alloc(skb);
>> +               tc_skb_ext = tc_skb_ext_alloc_napi(skb);
>>                 if (!tc_skb_ext) {
>>                         WARN_ON(1);
>>                         return false;
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> index 2d06b4412762..3d9da4ccaf5d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>> @@ -5643,7 +5643,7 @@ bool mlx5e_tc_update_skb(struct mlx5_cqe64 *cqe,
>>
>>         if (mapped_obj.type == MLX5_MAPPED_OBJ_CHAIN) {
>>                 chain = mapped_obj.chain;
>> -               tc_skb_ext = tc_skb_ext_alloc(skb);
>> +               tc_skb_ext = tc_skb_ext_alloc_napi(skb);
>>                 if (WARN_ON(!tc_skb_ext))
>>                         return false;
>>
>> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>> index ace437c6754b..82821a3f8a8b 100644
>> --- a/include/net/pkt_cls.h
>> +++ b/include/net/pkt_cls.h
>> @@ -764,6 +764,15 @@ static inline struct tc_skb_ext *tc_skb_ext_alloc(struct sk_buff *skb)
>>                 memset(tc_skb_ext, 0, sizeof(*tc_skb_ext));
>>         return tc_skb_ext;
>>  }
>> +
>> +static inline struct tc_skb_ext *tc_skb_ext_alloc_napi(struct sk_buff *skb)
>> +{
>> +       struct tc_skb_ext *tc_skb_ext = napi_skb_ext_add(skb, TC_SKB_EXT);
>> +
>> +       if (tc_skb_ext)
>> +               memset(tc_skb_ext, 0, sizeof(*tc_skb_ext));
>> +       return tc_skb_ext;
>> +}
>>  #endif
>>
>
>Dumb question: Would this work with a consumer of the metadata past
>RPS stage? didnt look closely but assumed per cpu skb_ext because of
>the napi context - which will require a per cpu pointer to fetch it
>later.
>In P4TC we make heavy use of skb_ext and found it to be very pricey,

Why don't you use skb->cb internally in p4tc? Or does the skb leave p4tc
and arrive back again? When?


>so we ended making it per cpu - but that's post RPS so we are safe.
>
>cheers,
>jamal