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

["Jonathan Lemon" <jonathan.lemon@gmail.com>]

On 4 Jun 2019, at 8:04, Maciej Fijalkowski 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.

I use the library for setting up all the ring bookkeeping, but use my 
own
application (and xdp program).  So I'd prefer not having the library do 
this -
as Björn notes, for some cases, the fill ring may not be populated.  I 
also
have some other use cases where I may not want to populate the fill ring 
until
later.


While convenient, I think there's a limit to what the library should be 
doing
for the user.
-- 
Jonathan