On Tue, 4 Jun 2019 10:06:36 +0200
Björn Töpel <bjorn.topel@xxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Krzysztof Kazimierczak <krzysztof.kazimierczak@xxxxxxxxx>
> > ---
> > 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.
> > sxdp.sxdp_family = PF_XDP;
> > sxdp.sxdp_ifindex = xsk->ifindex;
> > sxdp.sxdp_queue_id = xsk->queue_id;
> >
