| Message ID | 20250205190131.564456-3-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: improve core queue API handling while device is down | expand |
On Wed, Feb 5, 2025 at 11:02 AM Jakub Kicinski <kuba@kernel.org> wrote: > > 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. > > This is not a bug fix, because existing drivers check if > the interface is down as part of the ops. But new drivers > shouldn't have to do this, as long as they can correctly > alloc/free while down. > > 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 b02bb9f109d5..73d3401261a6 100644 > --- a/include/net/netdev_queues.h > +++ b/include/net/netdev_queues.h > @@ -117,6 +117,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 a5813d50e058..5352e0c1f37e 100644 > --- a/net/core/netdev_rx_queue.c > +++ b/net/core/netdev_rx_queue.c > @@ -37,13 +37,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); > + } Why not return an error if !netif_running(), and change the call site in net_devmem_unbind_dmabuf() to not call into this if !netif_running()? Is that a bit cleaner? It feels a bit weird to have netdev_rx_queue_restart() do a bunch of allocations and driver calls unnecessarily when it's really not going to do anything, no?
diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h index b02bb9f109d5..73d3401261a6 100644 --- a/include/net/netdev_queues.h +++ b/include/net/netdev_queues.h @@ -117,6 +117,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 a5813d50e058..5352e0c1f37e 100644 --- a/net/core/netdev_rx_queue.c +++ b/net/core/netdev_rx_queue.c @@ -37,13 +37,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);
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. This is not a bug fix, because existing drivers check if the interface is down as part of the ops. But new drivers shouldn't have to do this, as long as they can correctly alloc/free while down. 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(-)