netdev - Re: [PATCH net-next 3/3] net: create and use NAPI version of tc_skb_ext

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: <Y+0QTUQUDG7Zo1xq@nanopsycho>
Date:   Wed, 15 Feb 2023 18:03:09 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net,
        netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        willemb@...gle.com, fw@...len.de, saeedm@...dia.com,
        leon@...nel.org, xiyou.wangcong@...il.com, roid@...dia.com,
        ozsh@...dia.com, paulb@...dia.com
Subject: Re: [PATCH net-next 3/3] net: create and use NAPI version of
 tc_skb_ext_alloc()

Wed, Feb 15, 2023 at 05:50:55PM CET, jhs@...atatu.com wrote:
>On Tue, Feb 14, 2023 at 10:44 PM Jakub Kicinski <kuba@...nel.org> wrote:
>>
>> Try to use the cached skb_ext in the drivers.
>>
>> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>> ---
>> CC: saeedm@...dia.com
>> CC: leon@...nel.org
>> CC: jhs@...atatu.com
>> CC: xiyou.wangcong@...il.com
>> CC: jiri@...nulli.us
>> CC: roid@...dia.com
>> CC: ozsh@...dia.com
>> CC: paulb@...dia.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