Re: [PATCH v5 bpf-next 03/14] xdp: add xdp_shared_info data structure

From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
To: Shay Agroskin <shayagr@amazon.com>
Cc: Saeed Mahameed <saeed@kernel.org>,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	BPF-dev-list <bpf@vger.kernel.org>,
	Network Development <netdev@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	"Jubran, Samih" <sameehj@amazon.com>,
	John Fastabend <john.fastabend@gmail.com>,
	David Ahern <dsahern@kernel.org>,
	Jesper Brouer <brouer@redhat.com>,
	Eelco Chaudron <echaudro@redhat.com>,
	Jason Wang <jasowang@redhat.com>
Subject: Re: [PATCH v5 bpf-next 03/14] xdp: add xdp_shared_info data structure
Date: Sun, 20 Dec 2020 18:52:29 +0100	[thread overview]
Message-ID: <CAJ0CqmWUJzrpOpQ01sr+e5hr1K1U4tsqEiF=FdLL--wLYpu3DA@mail.gmail.com> (raw)
In-Reply-To: <pj41zlk0tdq22i.fsf@u68c7b5b1d2d758.ant.amazon.com>

>
>
> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:
>
> >> On Mon, 2020-12-07 at 17:32 +0100, Lorenzo Bianconi wrote:
> >> > Introduce xdp_shared_info data structure to contain info
> >> > about
> >> > "non-linear" xdp frame. xdp_shared_info will alias
> >> > skb_shared_info
> >> > allowing to keep most of the frags in the same cache-line.
> [...]
> >>
> >> > +  u16 nr_frags;
> >> > +  u16 data_length; /* paged area length */
> >> > +  skb_frag_t frags[MAX_SKB_FRAGS];
> >>
> >> why MAX_SKB_FRAGS ? just use a flexible array member
> >> skb_frag_t frags[];
> >>
> >> and enforce size via the n_frags and on the construction of the
> >> tailroom preserved buffer, which is already being done.
> >>
> >> this is waste of unnecessary space, at lease by definition of
> >> the
> >> struct, in your use case you do:
> >> memcpy(frag_list, xdp_sinfo->frags, sizeof(skb_frag_t) *
> >> num_frags);
> >> And the tailroom space was already preserved for a full
> >> skb_shinfo.
> >> so i don't see why you need this array to be of a fixed
> >> MAX_SKB_FRAGS
> >> size.
> >
> > In order to avoid cache-misses, xdp_shared info is built as a
> > variable
> > on mvneta_rx_swbm() stack and it is written to "shared_info"
> > area only on the
> > last fragment in mvneta_swbm_add_rx_fragment(). I used
> > MAX_SKB_FRAGS to be
> > aligned with skb_shared_info struct but probably we can use even
> > a smaller value.
> > Another approach would be to define two different struct, e.g.
> >
> > stuct xdp_frag_metadata {
> >       u16 nr_frags;
> >       u16 data_length; /* paged area length */
> > };
> >
> > struct xdp_frags {
> >       skb_frag_t frags[MAX_SKB_FRAGS];
> > };
> >
> > and then define xdp_shared_info as
> >
> > struct xdp_shared_info {
> >       stuct xdp_frag_metadata meta;
> >       skb_frag_t frags[];
> > };
> >
> > In this way we can probably optimize the space. What do you
> > think?
>
> We're still reserving ~sizeof(skb_shared_info) bytes at the end of
> the first buffer and it seems like in mvneta code you keep
> updating all three fields (frags, nr_frags and data_length).
> Can you explain how the space is optimized by splitting the
> structs please?

using xdp_shared_info struct we will have the first 3 fragments in the
same cacheline of nr_frags while using skb_shared_info struct only the
first fragment will be in the same cacheline of nr_frags. Moreover
skb_shared_info has multiple fields unused by xdp.

Regards,
Lorenzo

>
> >>
> >> > +};
> >> > +
> >> > +static inline struct xdp_shared_info *
> >> >  xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
> >> >  {
> >> > -  return (struct skb_shared_info *)xdp_data_hard_end(xdp);
> >> > +  BUILD_BUG_ON(sizeof(struct xdp_shared_info) >
> >> > +               sizeof(struct skb_shared_info));
> >> > +  return (struct xdp_shared_info *)xdp_data_hard_end(xdp);
> >> > +}
> >> > +
> >>
> >> Back to my first comment, do we have plans to use this tail
> >> room buffer
> >> for other than frag_list use cases ? what will be the buffer
> >> format
> >> then ? should we push all new fields to the end of the
> >> xdp_shared_info
> >> struct ? or deal with this tailroom buffer as a stack ?
> >> my main concern is that for drivers that don't support frag
> >> list and
> >> still want to utilize the tailroom buffer for other usecases
> >> they will
> >> have to skip the first sizeof(xdp_shared_info) so they won't
> >> break the
> >> stack.
> >
> > for the moment I do not know if this area is used for other
> > purposes.
> > Do you think there are other use-cases for it?
> >
>
> Saeed, the stack receives skb_shared_info when the frames are
> passed to the stack (skb_add_rx_frag is used to add the whole
> information to skb's shared info), and for XDP_REDIRECT use case,
> it doesn't seem like all drivers check page's tailroom for more
> information anyway (ena doesn't at least).
> Can you please explain what do you mean by "break the stack"?
>
> Thanks, Shay
>
> >>
> [...]
> >
> >>
>