Re: [RFC PATCH bpf-next 1/4] libbpf: fill the AF_XDP fill queue before bind() call

["Björn Töpel" <bjorn.topel@gmail.com>]

On Tue, 4 Jun 2019 at 17:06, Maciej Fijalkowski
<maciejromanfijalkowski@gmail.com> wrote:
>
> On Tue, 4 Jun 2019 10:06:36 +0200
> Björn Töpel <bjorn.topel@intel.com> wrote:
>
> > On 2019-06-03 15:19, Maciej Fijalkowski wrote:
> > > Let's get into the driver via ndo_bpf with command set to XDP_SETUP_UMEM
> > > with fill queue that already contains some available entries that can be
> > > used by Rx driver rings. Things worked in such way on old version of
> > > xdpsock (that lacked libbpf support) and there's no particular reason
> > > for having this preparation done after bind().
> > >
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > Signed-off-by: Krzysztof Kazimierczak <krzysztof.kazimierczak@intel.com>
> > > ---
> > >   samples/bpf/xdpsock_user.c | 15 ---------------
> > >   tools/lib/bpf/xsk.c        | 19 ++++++++++++++++++-
> > >   2 files changed, 18 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/samples/bpf/xdpsock_user.c b/samples/bpf/xdpsock_user.c
> > > index d08ee1ab7bb4..e9dceb09b6d1 100644
> > > --- a/samples/bpf/xdpsock_user.c
> > > +++ b/samples/bpf/xdpsock_user.c
> > > @@ -296,8 +296,6 @@ static struct xsk_socket_info *xsk_configure_socket(struct xsk_umem_info *umem)
> > >     struct xsk_socket_config cfg;
> > >     struct xsk_socket_info *xsk;
> > >     int ret;
> > > -   u32 idx;
> > > -   int i;
> > >
> > >     xsk = calloc(1, sizeof(*xsk));
> > >     if (!xsk)
> > > @@ -318,19 +316,6 @@ static struct xsk_socket_info *xsk_configure_socket(struct xsk_umem_info *umem)
> > >     if (ret)
> > >             exit_with_error(-ret);
> > >
> > > -   ret = xsk_ring_prod__reserve(&xsk->umem->fq,
> > > -                                XSK_RING_PROD__DEFAULT_NUM_DESCS,
> > > -                                &idx);
> > > -   if (ret != XSK_RING_PROD__DEFAULT_NUM_DESCS)
> > > -           exit_with_error(-ret);
> > > -   for (i = 0;
> > > -        i < XSK_RING_PROD__DEFAULT_NUM_DESCS *
> > > -                XSK_UMEM__DEFAULT_FRAME_SIZE;
> > > -        i += XSK_UMEM__DEFAULT_FRAME_SIZE)
> > > -           *xsk_ring_prod__fill_addr(&xsk->umem->fq, idx++) = i;
> > > -   xsk_ring_prod__submit(&xsk->umem->fq,
> > > -                         XSK_RING_PROD__DEFAULT_NUM_DESCS);
> > > -
> > >     return xsk;
> > >   }
> > >
> > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > > index 38667b62f1fe..57dda1389870 100644
> > > --- a/tools/lib/bpf/xsk.c
> > > +++ b/tools/lib/bpf/xsk.c
> > > @@ -529,7 +529,8 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
> > >     struct xdp_mmap_offsets off;
> > >     struct xsk_socket *xsk;
> > >     socklen_t optlen;
> > > -   int err;
> > > +   int err, i;
> > > +   u32 idx;
> > >
> > >     if (!umem || !xsk_ptr || !rx || !tx)
> > >             return -EFAULT;
> > > @@ -632,6 +633,22 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
> > >     }
> > >     xsk->tx = tx;
> > >
> > > +   err = xsk_ring_prod__reserve(umem->fill,
> > > +                                XSK_RING_PROD__DEFAULT_NUM_DESCS,
> > > +                                &idx);
> > > +   if (err != XSK_RING_PROD__DEFAULT_NUM_DESCS) {
> > > +           err = -errno;
> > > +           goto out_mmap_tx;
> > > +   }
> > > +
> > > +   for (i = 0;
> > > +        i < XSK_RING_PROD__DEFAULT_NUM_DESCS *
> > > +                XSK_UMEM__DEFAULT_FRAME_SIZE;
> > > +        i += XSK_UMEM__DEFAULT_FRAME_SIZE)
> > > +           *xsk_ring_prod__fill_addr(umem->fill, idx++) = i;
> > > +   xsk_ring_prod__submit(umem->fill,
> > > +                         XSK_RING_PROD__DEFAULT_NUM_DESCS);
> > > +
> >
> > Here, entries are added to the umem fill ring regardless if Rx is being
> > used or not. For a Tx only setup, this is not what we want, right?
>
> Right, but we have such behavior even without this patch. So I see two options
> here:
> - if you agree with this patch, then I guess we would need to pass the info to
>   libbpf what exactly we are setting up (txonly, rxdrop, l2fwd)?
> - otherwise, we should be passing the opt_bench onto xsk_configure_socket and
>   based on that decide whether we fill the fq or not?
>
> >
> > Thinking out loud here; Now libbpf is making the decision which umem
> > entries that are added to the fill ring. The sample application has this
> > (naive) scheme. I'm not sure that all applications would like that
> > policy. What do you think?
> >
>
> I find it convenient to have the fill queue in "initialized" state if I am
> making use of it, especially in case when I am doing the ZC so I must give the
> buffers to the driver via fill queue. So why would we bother other applications
> to provide it? I must admit that I haven't used AF_XDP with other apps than the
> example one, so I might not be able to elaborate further. Maybe other people
> have different feelings about it.
>

Personally, I think this scheme is not worth pursuing. I'd just leave
the fill ring work to the application. E.g. DPDK would definitely not
use a scheme like this.

Björn

> > >     sxdp.sxdp_family = PF_XDP;
> > >     sxdp.sxdp_ifindex = xsk->ifindex;
> > >     sxdp.sxdp_queue_id = xsk->queue_id;
> > >
>