Re: [PATCH net-next v1 07/11] xsk: Deliver provider chunks without copying

["Björn Töpel" <bjorn@kernel.org>]

Stanislav Fomichev <sdf.kernel@gmail.com> writes:

> On 11/10, Björn Töpel wrote:

>> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
>> index 113b0b08356d..b419c673e3f1 100644
>> --- a/net/xdp/xsk.c
>> +++ b/net/xdp/xsk.c

>> @@ -453,8 +501,14 @@ static int xsk_rcv_check(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
>>  
>>  static void xsk_flush(struct xdp_sock *xs)
>>  {
>> +	struct xsk_pp *pp = xs->pool->pp;
>> +
>> +	if (pp)
>> +		xsk_pp_flush(pp);
>>  	xskq_prod_submit(xs->rx);
>> -	__xskq_cons_release(xs->pool->fq);
>> +	/* A provider releases FILL entries under its lock. */
>> +	if (!pp)
>> +		__xskq_cons_release(xs->pool->fq);
>>  	sock_def_readable(&xs->sk);
>
> optional nit:
>
> if (pp) {
> 	pp_flush
> 	prod_submit
> } else {
> 	prod_submit
> 	cons_release
> }
>
> sock_def_readable
>
> (hard to follow these scattered if(pp) checks :-( )

Sure, will do!

>
>>  }
>>  
>> @@ -464,6 +518,11 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
>>  	int err;
>>  
>>  	err = xsk_rcv_check(xs, xdp, len);
>> +	/* Only the NAPI of the queue feeds a memory provider socket. */
>> +	if (!err && xs->pool->pp) {
>> +		xs->rx_dropped++;
>> +		err = -EOPNOTSUPP;
>> +	}
>
> This is confusing? Why even call xsk_rcv_check for pp case?

Hmm! I think you're right here... this was a (is it bound) check. I
might have been wrong in my reply to Maciej as well. I'll drop this.

>>  	if (!err) {
>>  		spin_lock_bh(&xs->pool->rx_lock);
>>  		err = __xsk_rcv(xs, xdp, len);
>> @@ -488,7 +547,26 @@ static int xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
>>  		return xsk_rcv_zc(xs, xdp, len);
>>  	}
>>  
>> -	err = __xsk_rcv(xs, xdp, len);
>> +	if (!xs->pool->pp) {
>> +		err = __xsk_rcv(xs, xdp, len);
>> +	} else {
>
> [..]
>
>> +		/* A CPUMAP program runs XDP on another CPU, not in NAPI. */
>> +		if (unlikely(!xdp_rxq_info_is_reg(xdp->rxq))) {
>> +			xs->rx_dropped++;
>> +			return -EINVAL;
>> +		}
>> +
>> +		err = xsk_pp_rcv_zc(xs->pool->pp, xs, xdp);
>> +		if (err != -EXDEV)
>> +			return err;
>> +
>> +		/* Copy only packets of the queue, from its NAPI */
>> +		if (unlikely(!xsk_pp_queue_buf(xs->pool->pp, xdp))) {
>> +			xs->rx_dropped++;
>> +			return -EINVAL;
>> +		}
>> +		err = xsk_pp_rcv_copy(xs, xdp, len);
>
> Move this into __xsk_rcv_pp ?

Yes.


Björn