Re: [PATCH 04/11] io_uring: setup ZC for an RX queue when registering an ifq

[David Wei <dw@davidwei.uk>]

On 25/08/2023 19:26, David Ahern wrote:
> On 8/25/23 6:19 PM, David Wei wrote:
>> diff --git a/io_uring/zc_rx.c b/io_uring/zc_rx.c
>> index 6c57c9b06e05..8cc66731af5b 100644
>> --- a/io_uring/zc_rx.c
>> +++ b/io_uring/zc_rx.c
>> @@ -10,6 +11,35 @@
>>  #include "kbuf.h"
>>  #include "zc_rx.h"
>>  
>> +typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf *bpf);
>> +
>> +static int __io_queue_mgmt(struct net_device *dev, struct io_zc_rx_ifq *ifq,
>> +			   u16 queue_id)
>> +{
>> +	struct netdev_bpf cmd;
>> +	bpf_op_t ndo_bpf;
>> +
>> +	ndo_bpf = dev->netdev_ops->ndo_bpf;
> 
> This is not bpf related, so it seems wrong to be overloading this ndo.

Agreed. I believe the original author (Jonathan) was inspired by
XDP_SETUP_XSK_POOL. I would also prefer a better way of setting up an RX queue
for ZC. Mina's proposal I think uses netlink for this.

Do you have any other suggestions? We want to keep resource registration
inline, so we need to be able to call it from within io_uring.

> 
> 
>> +	if (!ndo_bpf)
>> +		return -EINVAL;
>> +
>> +	cmd.command = XDP_SETUP_ZC_RX;
>> +	cmd.zc_rx.ifq = ifq;
>> +	cmd.zc_rx.queue_id = queue_id;
>> +
>> +	return ndo_bpf(dev, &cmd);
>> +}
>> +
>> +static int io_open_zc_rxq(struct io_zc_rx_ifq *ifq)
>> +{
>> +	return __io_queue_mgmt(ifq->dev, ifq, ifq->if_rxq_id);
>> +}
>> +
>> +static int io_close_zc_rxq(struct io_zc_rx_ifq *ifq)
>> +{
>> +	return __io_queue_mgmt(ifq->dev, NULL, ifq->if_rxq_id);
>> +}
>> +
>>  static struct io_zc_rx_ifq *io_zc_rx_ifq_alloc(struct io_ring_ctx *ctx)
>>  {
>>  	struct io_zc_rx_ifq *ifq;
>> @@ -19,12 +49,17 @@ static struct io_zc_rx_ifq *io_zc_rx_ifq_alloc(struct io_ring_ctx *ctx)
>>  		return NULL;
>>  
>>  	ifq->ctx = ctx;
>> +	ifq->if_rxq_id = -1;
>>  
>>  	return ifq;
>>  }
>>  
>>  static void io_zc_rx_ifq_free(struct io_zc_rx_ifq *ifq)
>>  {
>> +	if (ifq->if_rxq_id != -1)
>> +		io_close_zc_rxq(ifq);
>> +	if (ifq->dev)
>> +		dev_put(ifq->dev);
>>  	io_free_rbuf_ring(ifq);
>>  	kfree(ifq);
>>  }
>> @@ -41,17 +76,22 @@ int io_register_zc_rx_ifq(struct io_ring_ctx *ctx,
>>  		return -EFAULT;
>>  	if (ctx->ifq)
>>  		return -EBUSY;
>> +	if (reg.if_rxq_id == -1)
>> +		return -EINVAL;
>>  
>>  	ifq = io_zc_rx_ifq_alloc(ctx);
>>  	if (!ifq)
>>  		return -ENOMEM;
>>  
>> -	/* TODO: initialise network interface */
>> -
>>  	ret = io_allocate_rbuf_ring(ifq, &reg);
>>  	if (ret)
>>  		goto err;
>>  
>> +	ret = -ENODEV;
>> +	ifq->dev = dev_get_by_index(&init_net, reg.if_idx);
> 
> What's the plan for other namespaces? Ideally the device bind comes from
> a socket and that gives you the namespace.

Sorry, I did not consider namespaces yet. I'll look into how namespaces work
and then get back to you.

> 
>> +	if (!ifq->dev)
>> +		goto err;
>> +
>>  	/* TODO: map zc region and initialise zc pool */
>>  
>>  	ifq->rq_entries = reg.rq_entries;
> 
>