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: <CAM0EoMnkcvHpbJY-Tqo8CngN4Y_hnYoeaYCMW+OVXcNVAbwzug@mail.gmail.com>
Date:   Wed, 15 Feb 2023 11:50:55 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     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,
        jiri@...nulli.us, 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()

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,
so we ended making it per cpu - but that's post RPS so we are safe.

cheers,
jamal