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]

Date:   Wed, 15 Feb 2023 13:36:02 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jiri Pirko <jiri@...nulli.us>
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()

On Wed, Feb 15, 2023 at 12:03 PM Jiri Pirko <jiri@...nulli.us> wrote:
>
> 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?

Just for space reasons since we have a lot of stuff we carry around
(headers, metadata, and table keys which could be each upto an imposed
512b in size).

> Or does the skb leave p4tc and arrive back again? When?

Only time it leaves is if we do a "recirculate" - which is essentially
a mirred redirect to ingress/egress of another port.
It still causes memory pressure in alloc/free - we are thinking of
getting rid of it altogether and replacing it with a statically
allocated per-cpu scratchpad; if it gets recirculated we start from
scratch.

cheers,
jamal