Re: [PATCH v5 bpf-next 11/14] bpf: cpumap: introduce xdp multi-buff support

From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
To: Shay Agroskin <shayagr@amazon.com>
Cc: 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 11/14] bpf: cpumap: introduce xdp multi-buff support
Date: Sun, 20 Dec 2020 18:56:07 +0100	[thread overview]
Message-ID: <CAJ0CqmXB2fBoUHgOcpXOkEhfV2Bp=Np+2rhKSOCaZiFdz4WZPQ@mail.gmail.com> (raw)
In-Reply-To: <pj41zleejlpu3c.fsf@u68c7b5b1d2d758.ant.amazon.com>

>
>
> Lorenzo Bianconi <lorenzo@kernel.org> writes:
>
> > Introduce __xdp_build_skb_from_frame and
> > xdp_build_skb_from_frame
> > utility routines to build the skb from xdp_frame.
> > Add xdp multi-buff support to cpumap
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  include/net/xdp.h   |  5 ++++
> >  kernel/bpf/cpumap.c | 45 +---------------------------
> >  net/core/xdp.c      | 73
> >  +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 79 insertions(+), 44 deletions(-)
> >
> [...]
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 6c8e743ad03a..55f3e9c69427 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -597,3 +597,76 @@ void xdp_warn(const char *msg, const char
> > *func, const int line)
> >       WARN(1, "XDP_WARN: %s(line:%d): %s\n", func, line, msg);
> >  };
> >  EXPORT_SYMBOL_GPL(xdp_warn);
> > +
> > +struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame
> > *xdpf,
> > +                                        struct sk_buff *skb,
> > +                                        struct net_device *dev)
> > +{
> > +     unsigned int headroom = sizeof(*xdpf) + xdpf->headroom;
> > +     void *hard_start = xdpf->data - headroom;
> > +     skb_frag_t frag_list[MAX_SKB_FRAGS];
> > +     struct xdp_shared_info *xdp_sinfo;
> > +     int i, num_frags = 0;
> > +
> > +     xdp_sinfo = xdp_get_shared_info_from_frame(xdpf);
> > +     if (unlikely(xdpf->mb)) {
> > +             num_frags = xdp_sinfo->nr_frags;
> > +             memcpy(frag_list, xdp_sinfo->frags,
> > +                    sizeof(skb_frag_t) * num_frags);
> > +     }
>
> nit, can you please move the xdp_sinfo assignment inside this 'if'
> ? This would help to emphasize that regarding xdp_frame tailroom
> as xdp_shared_info struct (rather than skb_shared_info) is correct
> only when the mb bit is set
>
> thanks,
> Shay

ack, will do in v6.

Regards,
Lorenzo

>
> > +
> > +     skb = build_skb_around(skb, hard_start, xdpf->frame_sz);
> > +     if (unlikely(!skb))
> > +             return NULL;
> [...]
>