[PATCH net-next 0/3] net: improve core queue API handling while device is down

[PATCH net-next 0/3] net: improve core queue API handling while device is down

[Jakub Kicinski]

The core netdev_rx_queue_restart() doesn't currently take into account
that the device may be down. The current and proposed queue API
implementations deal with this by rejecting queue API calls while
the device is down. We can do better, in theory we can still allow
devmem binding when the device is down - we shouldn't stop and start
the queues just try to allocate the memory. The reason we allocate
the memory is that memory provider binding checks if any compatible
page pool has been created (page_pool_check_memory_provider()).

Previously I thought we need this as a fix, but gve rejects page pool
calls while down, and so did Saeed in the patches he posted. So this
series just makes the core act more sensibly but practically should
be a noop for now.

Jakub Kicinski (3):
  net: refactor netdev_rx_queue_restart() to use local qops
  net: devmem: don't call queue stop / start when the interface is down
  netdevsim: allow normal queue reset while down

 include/net/netdev_queues.h              |  4 +++
 drivers/net/netdevsim/netdev.c           |  8 ++---
 net/core/netdev_rx_queue.c               | 37 +++++++++++++-----------
 tools/testing/selftests/net/nl_netdev.py | 17 ++++++++++-
 4 files changed, 43 insertions(+), 23 deletions(-)

-- 
2.48.1

[PATCH net-next 1/3] net: refactor netdev_rx_queue_restart() to use local qops

[Jakub Kicinski]

Shorten the lines by storing dev->queue_mgmt_ops in a temp variable.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/netdev_rx_queue.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
index db82786fa0c4..a5813d50e058 100644
--- a/net/core/netdev_rx_queue.c
+++ b/net/core/netdev_rx_queue.c
@@ -9,28 +9,27 @@
 int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
 {
 	struct netdev_rx_queue *rxq = __netif_get_rx_queue(dev, rxq_idx);
+	const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
 	void *new_mem, *old_mem;
 	int err;
 
-	if (!dev->queue_mgmt_ops || !dev->queue_mgmt_ops->ndo_queue_stop ||
-	    !dev->queue_mgmt_ops->ndo_queue_mem_free ||
-	    !dev->queue_mgmt_ops->ndo_queue_mem_alloc ||
-	    !dev->queue_mgmt_ops->ndo_queue_start)
+	if (!qops || !qops->ndo_queue_stop || !qops->ndo_queue_mem_free ||
+	    !qops->ndo_queue_mem_alloc || !qops->ndo_queue_start)
 		return -EOPNOTSUPP;
 
 	ASSERT_RTNL();
 
-	new_mem = kvzalloc(dev->queue_mgmt_ops->ndo_queue_mem_size, GFP_KERNEL);
+	new_mem = kvzalloc(qops->ndo_queue_mem_size, GFP_KERNEL);
 	if (!new_mem)
 		return -ENOMEM;
 
-	old_mem = kvzalloc(dev->queue_mgmt_ops->ndo_queue_mem_size, GFP_KERNEL);
+	old_mem = kvzalloc(qops->ndo_queue_mem_size, GFP_KERNEL);
 	if (!old_mem) {
 		err = -ENOMEM;
 		goto err_free_new_mem;
 	}
 
-	err = dev->queue_mgmt_ops->ndo_queue_mem_alloc(dev, new_mem, rxq_idx);
+	err = qops->ndo_queue_mem_alloc(dev, new_mem, rxq_idx);
 	if (err)
 		goto err_free_old_mem;
 
@@ -38,15 +37,15 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
 	if (err)
 		goto err_free_new_queue_mem;
 
-	err = dev->queue_mgmt_ops->ndo_queue_stop(dev, old_mem, rxq_idx);
+	err = qops->ndo_queue_stop(dev, old_mem, rxq_idx);
 	if (err)
 		goto err_free_new_queue_mem;
 
-	err = dev->queue_mgmt_ops->ndo_queue_start(dev, new_mem, rxq_idx);
+	err = qops->ndo_queue_start(dev, new_mem, rxq_idx);
 	if (err)
 		goto err_start_queue;
 
-	dev->queue_mgmt_ops->ndo_queue_mem_free(dev, old_mem);
+	qops->ndo_queue_mem_free(dev, old_mem);
 
 	kvfree(old_mem);
 	kvfree(new_mem);
@@ -61,15 +60,15 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
 	 * WARN if we fail to recover the old rx queue, and at least free
 	 * old_mem so we don't also leak that.
 	 */
-	if (dev->queue_mgmt_ops->ndo_queue_start(dev, old_mem, rxq_idx)) {
+	if (qops->ndo_queue_start(dev, old_mem, rxq_idx)) {
 		WARN(1,
 		     "Failed to restart old queue in error path. RX queue %d may be unhealthy.",
 		     rxq_idx);
-		dev->queue_mgmt_ops->ndo_queue_mem_free(dev, old_mem);
+		qops->ndo_queue_mem_free(dev, old_mem);
 	}
 
 err_free_new_queue_mem:
-	dev->queue_mgmt_ops->ndo_queue_mem_free(dev, new_mem);
+	qops->ndo_queue_mem_free(dev, new_mem);
 
 err_free_old_mem:
 	kvfree(old_mem);
-- 
2.48.1

Re: [PATCH net-next 1/3] net: refactor netdev_rx_queue_restart() to use local qops

[Mina Almasry]

On Wed, Feb 5, 2025 at 11:01 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Shorten the lines by storing dev->queue_mgmt_ops in a temp variable.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Mina Almasry <almasrymina@google.com>

[PATCH net-next 2/3] net: devmem: don't call queue stop / start when the interface is down

[Jakub Kicinski]

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);
+	}
 
 	qops->ndo_queue_mem_free(dev, old_mem);
 
-- 
2.48.1

Re: [PATCH net-next 2/3] net: devmem: don't call queue stop / start when the interface is down

[Mina Almasry]

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?

-- 
Thanks,
Mina

Re: [PATCH net-next 2/3] net: devmem: don't call queue stop / start when the interface is down

[Jakub Kicinski]

On Wed, 5 Feb 2025 12:35:30 -0800 Mina Almasry wrote:
> 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?

The bindings survive ifdown, right? So presumably they exist while 
the device is down. If they exist while the device is down, why can
they not be created? Feels inconsistent.

Re: [PATCH net-next 2/3] net: devmem: don't call queue stop / start when the interface is down

[Mina Almasry]

On Wed, Feb 5, 2025 at 5:26 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Feb 2025 12:35:30 -0800 Mina Almasry wrote:
> > 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?
>
> The bindings survive ifdown, right? So presumably they exist while
> the device is down. If they exist while the device is down, why can
> they not be created? Feels inconsistent.

Ah, good point. I guess binding to a device that is down is WAI indeed.

Another approach could be to return success without doing any driver
calls at all then, because when the device is down there is no running
queue to restart. Although that would have an effect that devmem TCP
would start working on devices that don't support the queue API at all
(I guess the user could bring the interface down, do bindings, then
back up).

But this works too, so

Reviewed-by: Mina Almasry <almasrymina@google.com>

-- 
Thanks,
Mina

[PATCH net-next 3/3] netdevsim: allow normal queue reset while down

[Jakub Kicinski]

Resetting queues while the device is down should be legal.
Allow it, test it. Ideally we'd test this with a real device
supporting devmem but I don't have access to such devices.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/netdev.c           |  8 +++-----
 tools/testing/selftests/net/nl_netdev.py | 17 ++++++++++++++++-
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 42f247cbdcee..d26b2fb1cabc 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -644,6 +644,9 @@ nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx)
 
 	if (ns->rq_reset_mode > 3)
 		return -EINVAL;
+	/* Only "mode 0" works when device is down */
+	if (!netif_running(ns->netdev) && ns->rq_reset_mode)
+		return -ENETDOWN;
 
 	if (ns->rq_reset_mode == 1)
 		return nsim_create_page_pool(&qmem->pp, &ns->rq[idx]->napi);
@@ -754,11 +757,6 @@ nsim_qreset_write(struct file *file, const char __user *data,
 		return -EINVAL;
 
 	rtnl_lock();
-	if (!netif_running(ns->netdev)) {
-		ret = -ENETDOWN;
-		goto exit_unlock;
-	}
-
 	if (queue >= ns->netdev->real_num_rx_queues) {
 		ret = -EINVAL;
 		goto exit_unlock;
diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
index 93e8cb671c3d..9a03e3fe7e31 100755
--- a/tools/testing/selftests/net/nl_netdev.py
+++ b/tools/testing/selftests/net/nl_netdev.py
@@ -35,6 +35,20 @@ from lib.py import NetdevFamily, NetdevSimDev, ip
                         comment=f"queue count after reset queue {q} mode {i}")
 
 
+def nsim_rxq_reset_down(nf) -> None:
+    """
+    Test that the queue API supports resetting a queue
+    while the interface is down. We should convert this
+    test to testing real HW once more devices support
+    queue API.
+    """
+    with NetdevSimDev(queue_count=4) as nsimdev:
+        nsim = nsimdev.nsims[0]
+
+        ip(f"link set dev {nsim.ifname} down")
+        nsim.dfs_write("queue_reset", f"1 0")
+
+
 def page_pool_check(nf) -> None:
     with NetdevSimDev() as nsimdev:
         nsim = nsimdev.nsims[0]
@@ -106,7 +120,8 @@ from lib.py import NetdevFamily, NetdevSimDev, ip
 
 def main() -> None:
     nf = NetdevFamily()
-    ksft_run([empty_check, lo_check, page_pool_check, napi_list_check],
+    ksft_run([empty_check, lo_check, page_pool_check, napi_list_check,
+              nsim_rxq_reset_down],
              args=(nf, ))
     ksft_exit()
 
-- 
2.48.1

Re: [PATCH net-next 3/3] netdevsim: allow normal queue reset while down

[Mina Almasry]

On Wed, Feb 5, 2025 at 11:02 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Resetting queues while the device is down should be legal.
> Allow it, test it. Ideally we'd test this with a real device
> supporting devmem but I don't have access to such devices.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Mina Almasry <almasrymina@google.com>

> ---
>  drivers/net/netdevsim/netdev.c           |  8 +++-----
>  tools/testing/selftests/net/nl_netdev.py | 17 ++++++++++++++++-
>  2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 42f247cbdcee..d26b2fb1cabc 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -644,6 +644,9 @@ nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx)
>
>         if (ns->rq_reset_mode > 3)
>                 return -EINVAL;
> +       /* Only "mode 0" works when device is down */

Just to make sure I understand: modes 2 & 3 should also work with the
device down, and this is an artificial limitation that you're adding
to netdevsim, right? I don't see how changing the call order to
napi_del and napi_add_config breaks resetting while the device is
down.

Re: [PATCH net-next 3/3] netdevsim: allow normal queue reset while down

[Jakub Kicinski]

On Thu, 6 Feb 2025 08:43:58 -0800 Mina Almasry wrote:
> > diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> > index 42f247cbdcee..d26b2fb1cabc 100644
> > --- a/drivers/net/netdevsim/netdev.c
> > +++ b/drivers/net/netdevsim/netdev.c
> > @@ -644,6 +644,9 @@ nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx)
> >
> >         if (ns->rq_reset_mode > 3)
> >                 return -EINVAL;
> > +       /* Only "mode 0" works when device is down */  
> 
> Just to make sure I understand: modes 2 & 3 should also work with the
> device down, and this is an artificial limitation that you're adding
> to netdevsim, right? I don't see how changing the call order to
> napi_del and napi_add_config breaks resetting while the device is
> down.

It should work, but I sprinkled a bunch of checks in page pool which
expect idle NAPIs to have the SCHED bit set. So we run into false
positive warnings if we create and destroy a page pool for NAPI which
was never initialized / listed. Let me take another look.