On Tue, 4 Jun 2019 at 17:06, Maciej Fijalkowski
<maciejromanfijalkowski@xxxxxxxxx> wrote:
>
> 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.
>
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;
> > >
>
