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

[PATCH net-next v2 0/4] 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()).

Alternatively we could reject installing MP while the device is down
but the MP assignment survives ifdown (so presumably MP doesn't cease
to exist while down), and in general we allow configuration while down.

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.

v2:
 - allow also mode 2 and 3 (patch 3 is new)
v1: https://lore.kernel.org/20250205190131.564456-1-kuba@kernel.org

Jakub Kicinski (4):
  net: refactor netdev_rx_queue_restart() to use local qops
  net: devmem: don't call queue stop / start when the interface is down
  net: page_pool: avoid false positive warning if NAPI was never added
  netdevsim: allow normal queue reset while down

 include/net/netdev_queues.h              |  4 +++
 net/core/dev.h                           | 12 ++++++++
 drivers/net/netdevsim/netdev.c           | 10 +++----
 net/core/netdev_rx_queue.c               | 37 +++++++++++++-----------
 net/core/page_pool.c                     |  7 ++---
 tools/testing/selftests/net/nl_netdev.py | 18 +++++++++++-
 6 files changed, 59 insertions(+), 29 deletions(-)

-- 
2.48.1

[PATCH net-next v2 1/4] 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 v2 1/4] net: refactor netdev_rx_queue_restart() to use local qops

[Mina Almasry]

On Thu, Feb 6, 2025 at 2:56 PM 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>

--
Thanks,
Mina

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

[Jakub Kicinski]

On Thu, 6 Feb 2025 15:05:56 -0800 Mina Almasry wrote:
> On Thu, Feb 6, 2025 at 2:56 PM 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>

Ah, forgot to carry your tags forward, didn't I.. Sorry.

[PATCH net-next v2 2/4] 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 v2 2/4] net: devmem: don't call queue stop / start when the interface is down

[Mina Almasry]

On Thu, Feb 6, 2025 at 2:56 PM 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.
>

Gah, I think I made a suggestion on v1 that would have broken this
check. We do indeed need to make the driver calls, if only for the
check that the driver the memory provider enabled pool.

> 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>

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

[PATCH net-next v2 3/4] net: page_pool: avoid false positive warning if NAPI was never added

[Jakub Kicinski]

We expect NAPI to be in disabled state when page pool is torn down.
But it is also legal if the NAPI is completely uninitialized.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2: new patch
v1: https://lore.kernel.org/20250205190131.564456-1-kuba@kernel.org

CC: hawk@kernel.org
CC: ilias.apalodimas@linaro.org
CC: jdamato@fastly.com
---
 net/core/dev.h       | 12 ++++++++++++
 net/core/page_pool.c |  7 ++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.h b/net/core/dev.h
index a5b166bbd169..caa13e431a6b 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -299,6 +299,18 @@ void xdp_do_check_flushed(struct napi_struct *napi);
 static inline void xdp_do_check_flushed(struct napi_struct *napi) { }
 #endif
 
+/* Best effort check that NAPI is not idle (can't be scheduled to run) */
+static inline void napi_assert_will_not_race(const struct napi_struct *napi)
+{
+	/* uninitialized instance, can't race */
+	if (!napi->poll_list.next)
+		return;
+
+	/* SCHED bit is set on disabled instances */
+	WARN_ON(!test_bit(NAPI_STATE_SCHED, &napi->state));
+	WARN_ON(READ_ONCE(napi->list_owner) != -1);
+}
+
 void kick_defer_list_purge(struct softnet_data *sd, unsigned int cpu);
 
 #define XMIT_RECURSION_LIMIT	8
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index f5e908c9e7ad..b940c40ea2f2 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -25,6 +25,7 @@
 
 #include <trace/events/page_pool.h>
 
+#include "dev.h"
 #include "mp_dmabuf_devmem.h"
 #include "netmem_priv.h"
 #include "page_pool_priv.h"
@@ -1140,11 +1141,7 @@ void page_pool_disable_direct_recycling(struct page_pool *pool)
 	if (!pool->p.napi)
 		return;
 
-	/* To avoid races with recycling and additional barriers make sure
-	 * pool and NAPI are unlinked when NAPI is disabled.
-	 */
-	WARN_ON(!test_bit(NAPI_STATE_SCHED, &pool->p.napi->state));
-	WARN_ON(READ_ONCE(pool->p.napi->list_owner) != -1);
+	napi_assert_will_not_race(pool->p.napi);
 
 	mutex_lock(&page_pools_lock);
 	WRITE_ONCE(pool->p.napi, NULL);
-- 
2.48.1

Re: [PATCH net-next v2 3/4] net: page_pool: avoid false positive warning if NAPI was never added

[Mina Almasry]

On Thu, Feb 6, 2025 at 2:56 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> We expect NAPI to be in disabled state when page pool is torn down.
> But it is also legal if the NAPI is completely uninitialized.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

-- 
Thanks,
Mina

[PATCH net-next v2 4/4] 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           | 10 ++++------
 tools/testing/selftests/net/nl_netdev.py | 18 +++++++++++++++++-
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 42f247cbdcee..9b394ddc5206 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -645,8 +645,11 @@ nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx)
 	if (ns->rq_reset_mode > 3)
 		return -EINVAL;
 
-	if (ns->rq_reset_mode == 1)
+	if (ns->rq_reset_mode == 1) {
+		if (!netif_running(ns->netdev))
+			return -ENETDOWN;
 		return nsim_create_page_pool(&qmem->pp, &ns->rq[idx]->napi);
+	}
 
 	qmem->rq = nsim_queue_alloc();
 	if (!qmem->rq)
@@ -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..beaee5e4e2aa 100755
--- a/tools/testing/selftests/net/nl_netdev.py
+++ b/tools/testing/selftests/net/nl_netdev.py
@@ -35,6 +35,21 @@ 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")
+        for i in [0, 2, 3]:
+            nsim.dfs_write("queue_reset", f"1 {i}")
+
+
 def page_pool_check(nf) -> None:
     with NetdevSimDev() as nsimdev:
         nsim = nsimdev.nsims[0]
@@ -106,7 +121,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 v2 4/4] netdevsim: allow normal queue reset while down

[Mina Almasry]

On Thu, Feb 6, 2025 at 2:56 PM 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>
> ---
>  drivers/net/netdevsim/netdev.c           | 10 ++++------
>  tools/testing/selftests/net/nl_netdev.py | 18 +++++++++++++++++-
>  2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 42f247cbdcee..9b394ddc5206 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -645,8 +645,11 @@ nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx)
>         if (ns->rq_reset_mode > 3)
>                 return -EINVAL;
>
> -       if (ns->rq_reset_mode == 1)
> +       if (ns->rq_reset_mode == 1) {
> +               if (!netif_running(ns->netdev))
> +                       return -ENETDOWN;

To be honest I could not track down for myself why mode 1 needed to be
excluded as well. AFAICT it should be a similar story to modes 2/3,
i.e. we never initialize the napi on nsim_queue_mem_alloc() for all of
 modes [1, 2, 3] and this particular warning you fixed in patch 3
should again not fire for mode 1 just like they stopped firing for 2 &
3.

But, I'm not sure it's critical to expand the coverage further to mode
1, so FWIW

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


-- 
Thanks,
Mina

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  6 Feb 2025 14:56:34 -0800 you wrote:
> 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()).
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/4] net: refactor netdev_rx_queue_restart() to use local qops
    https://git.kernel.org/netdev/net-next/c/1eb824d69f8d
  - [net-next,v2,2/4] net: devmem: don't call queue stop / start when the interface is down
    https://git.kernel.org/netdev/net-next/c/3e7efc3f4f03
  - [net-next,v2,3/4] net: page_pool: avoid false positive warning if NAPI was never added
    https://git.kernel.org/netdev/net-next/c/c1e00bc4be06
  - [net-next,v2,4/4] netdevsim: allow normal queue reset while down
    https://git.kernel.org/netdev/net-next/c/285b3f78eabd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html