Re: [PATCH net-next v11 10/21] net: add helpers for setting a memory provider on an rx queue

[Pavel Begunkov <asml.silence@gmail.com>]

On 1/17/25 22:11, Jakub Kicinski wrote:
> On Fri, 17 Jan 2025 02:47:15 +0000 Pavel Begunkov wrote:
>>>> +	rxq = __netif_get_rx_queue(dev, ifq_idx);
>>>
>>> I think there's a small race between io_uring closing and the netdev
>>> unregister. We can try to uninstall twice, let's put
>>
>> They're gated by checking ifq->netdev in io_uring code, which is
>> cleared by them under a spin. So either io_uring does
>> __net_mp_close_rxq() and ->uninstall does nothing, or vise versa.
> 
> True, so not twice, but the race is there. It's not correct to call
> ops of a device which has already been unregistered.

Ok, from what you're saying it's regardless of the netdev still
having refs lingering. In this case it was better a version ago
where io_uring was just taking the rtnl lock, which protects
against concurrent unregistration while io_uring is checking
netdev.

Does your patch below covers that? Or does it have to be resolved
in this set? I assume you're going to queue it as a fix.

  
> Mina, did we consider that the device may be closed when the provider
> is being bound? Perhaps that's what you meant when you were reviewing
> the netdevsim patches!
> 
> Do we need something like this?
> 
> ---->8------------
> 
> From: Jakub Kicinski <kuba@kernel.org>
> Subject: net: devmem: don't call queue stop / start when the interface is down
> 
> We seem to be missing a netif_running() check from the devmem
> installation path. Starting a queue on a stopped device makes
> no sense. We still want to be able to allocate the memory, just
> to test that the device is indeed setting up the page pools
> in a memory provider compatible way.
> 
> Fixes: 7c88f86576f3 ("netdev: add netdev_rx_queue_restart()")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   include/net/netdev_queues.h |  4 ++++
>   net/core/netdev_rx_queue.c  | 16 ++++++++++------
>   2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index 5ca019d294ca..9296efeab4c0 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -107,6 +107,10 @@ struct netdev_stat_ops {
>    *
>    * @ndo_queue_stop:	Stop the RX queue at the specified index. The stopped
>    *			queue's memory is written at the specified address.
> + *
> + * Note that @ndo_queue_mem_alloc and @ndo_queue_mem_free may be called while
> + * the interface is closed. @ndo_queue_start and @ndo_queue_stop will only
> + * be called for an interface which is open.
>    */
>   struct netdev_queue_mgmt_ops {
>   	size_t			ndo_queue_mem_size;
> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
> index b02b28d2ae44..9b9c2589150a 100644
> --- a/net/core/netdev_rx_queue.c
> +++ b/net/core/netdev_rx_queue.c
> @@ -38,13 +38,17 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
>   	if (err)
>   		goto err_free_new_queue_mem;
>   
> -	err = qops->ndo_queue_stop(dev, old_mem, rxq_idx);
> -	if (err)
> -		goto err_free_new_queue_mem;
> +	if (netif_running(dev)) {
> +		err = qops->ndo_queue_stop(dev, old_mem, rxq_idx);
> +		if (err)
> +			goto err_free_new_queue_mem;
>   
> -	err = qops->ndo_queue_start(dev, new_mem, rxq_idx);
> -	if (err)
> -		goto err_start_queue;
> +		err = qops->ndo_queue_start(dev, new_mem, rxq_idx);
> +		if (err)
> +			goto err_start_queue;
> +	} else {
> +		swap(new_mem, old_mem);
> +	}
>   
>   	qops->ndo_queue_mem_free(dev, old_mem);
>   

-- 
Pavel Begunkov