Re: [PATCH v9 bpf-next 07/14] net: xdp: add multi-buff support to xdp_build_skb_from_frame

From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
To: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Lorenzo Bianconi <lorenzo@kernel.org>, bpf <bpf@vger.kernel.org>,
	Netdev <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	shayagr@amazon.com, "Jubran, Samih" <sameehj@amazon.com>,
	John Fastabend <john.fastabend@gmail.com>,
	David Ahern <dsahern@kernel.org>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	Eelco Chaudron <echaudro@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	Saeed Mahameed <saeed@kernel.org>,
	Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
	"Karlsson, Magnus" <magnus.karlsson@intel.com>,
	Tirthendu <tirthendu.sarkar@intel.com>
Subject: Re: [PATCH v9 bpf-next 07/14] net: xdp: add multi-buff support to xdp_build_skb_from_frame
Date: Tue, 29 Jun 2021 20:34:05 +0200	[thread overview]
Message-ID: <YNtnnT8kEgT5KdWF@lore-desk> (raw)
In-Reply-To: <CAKgT0Ue=74BPWFDFYtWEA9DnNj35PgigDZAwCc5N6X=QpKz4GA@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2284 bytes --]

> On Mon, Jun 14, 2021 at 5:51 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > Introduce xdp multi-buff support to
> > __xdp_build_skb_from_frame/xdp_build_skb_from_frame
> > utility routines.
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  net/core/xdp.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index f61c63115c95..71bedf6049a1 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -582,9 +582,15 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
> >                                            struct sk_buff *skb,
> >                                            struct net_device *dev)
> >  {
> > +       struct skb_shared_info *sinfo = xdp_get_shared_info_from_frame(xdpf);
> >         unsigned int headroom, frame_size;
> > +       int i, num_frags = 0;
> >         void *hard_start;
> >
> > +       /* xdp multi-buff frame */
> > +       if (unlikely(xdp_frame_is_mb(xdpf)))
> > +               num_frags = sinfo->nr_frags;
> > +
> >         /* Part of headroom was reserved to xdpf */
> >         headroom = sizeof(*xdpf) + xdpf->headroom;
> >
> > @@ -603,6 +609,13 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
> >         if (xdpf->metasize)
> >                 skb_metadata_set(skb, xdpf->metasize);
> >
> > +       for (i = 0; i < num_frags; i++)
> > +               skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> > +                               skb_frag_page(&sinfo->frags[i]),
> > +                               skb_frag_off(&sinfo->frags[i]),
> > +                               skb_frag_size(&sinfo->frags[i]),
> > +                               xdpf->frame_sz);
> > +
> 
> So this is assuming the header frame and all of the frags are using
> the same size. Rather than reading the frags out and then writing them
> back, why not just directly rewrite the nr_frags, add the total size
> to skb->len and skb->data_len, and then update the truesize?

ack, thx. I will look into it.

Regards,
Lorenzo

> 
> Actually, I think you might need to store the truesize somewhere in
> addition to the data_len that you were storing in the shared info.
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]