Re: [PATCH RESEND bpf-next 14/15] net, xdp: allow metadata > 32

[Jesper Dangaard Brouer]

On 19/05/2023 18.35, Alexander Lobakin wrote:
> From: Jesper Dangaard Brouer <jbrouer@xxxxxxxxxx>
> Date: Tue, 16 May 2023 17:35:27 +0200
>
> > On 16/05/2023 14.37, Alexander Lobakin wrote:
> > > From: Larysa Zaremba<larysa.zaremba@xxxxxxxxx>
> > > Date: Mon, 15 May 2023 19:08:39 +0200
> > >
> > > > On Mon, May 15, 2023 at 06:17:02PM +0200, Jesper Dangaard Brouer wrote:
> > > > > On 12/05/2023 17.26, Larysa Zaremba wrote:
> > > > > > From: Aleksander Lobakin<aleksander.lobakin@xxxxxxxxx>
> > > > > >
> > > > > > When using XDP hints, metadata sometimes has to be much bigger
> > > > > > than 32 bytes. Relax the restriction, allow metadata larger than 32
> > > > > > bytes
> > > > > > and make __skb_metadata_differs() work with bigger lengths.
> > > > > >
> > > > > > Now size of metadata is only limited by the fact it is stored as u8
> > > > > > in skb_shared_info, so maximum possible value is 255.
> > > > >
> > > > > I'm confused, IIRC the metadata area isn't stored "in skb_shared_info".
> > > > > The maximum possible size is limited by the XDP headroom, which is also
> > > > > shared/limited with/by xdp_frame.  I must be reading the sentence
> > > > > wrong,
> > > > > somehow.
> > >
> > > skb_shared_info::meta_size  is u8. Since metadata gets carried from
> > > xdp_buff to skb, this check is needed (it's compile-time constant
> > > anyway).
> > > Check for headroom is done separately already (two sentences below).
> >
> > Damn, argh, for SKBs the "meta_len" is stored in skb_shared_info, which
> > is located on another cacheline.
> > That is a sure way to KILL performance! :-(
>
> Have you read the code? I use type_max(typeof_member(shinfo, meta_len)),
> what performance are you talking about?

Not talking about your changes (in this patch).

I'm realizing that SKBs using metadata area will have a performance hit
due to accessing another cacheline (the meta_len in skb_shared_info).

IIRC Daniel complained about this performance hit (in the past), I guess
this explains it.  IIRC Cilium changed to use percpu variables/datastore
to workaround this.


> The whole xdp_metalen_invalid() gets expanded into:
>
> 	return (metalen % 4) || metalen > 255;
>
> at compile-time. All those typeof shenanigans are only to not open-code
> meta_len's type/size/max.
>
> > But only use for SKBs that gets created from xdp with metadata, right?

Normal netstack processing actually access this skb_shinfo->meta_len in
gro_list_prepare().  As the caller dev_gro_receive() later access other
memory in skb_shared_info, then the GRO code path already takes this hit
to begin with.

--Jesper