* [PATCH net v2 1/2] net: move mp dev config validation to __net_mp_open_rxq()
2025-04-03 1:34 [PATCH net v2 0/2] net: make memory provider install / close paths more common Jakub Kicinski
@ 2025-04-03 1:34 ` Jakub Kicinski
2025-04-03 1:34 ` [PATCH net v2 2/2] net: avoid false positive warnings in __net_mp_close_rxq() Jakub Kicinski
2025-04-04 15:10 ` [PATCH net v2 0/2] net: make memory provider install / close paths more common patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2025-04-03 1:34 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, andrew+netdev, horms, ap420073,
almasrymina, asml.silence, dw, sdf, Jakub Kicinski
devmem code performs a number of safety checks to avoid having
to reimplement all of them in the drivers. Move those to
__net_mp_open_rxq() and reuse that function for binding to make
sure that io_uring ZC also benefits from them.
While at it rename the queue ID variable to rxq_idx in
__net_mp_open_rxq(), we touch most of the relevant lines.
The XArray insertion is reordered after the netdev_rx_queue_restart()
call, otherwise we'd need to duplicate the queue index check
or risk inserting an invalid pointer. The XArray allocation
failures should be extremely rare.
Reviewed-by: Mina Almasry <almasrymina@google.com>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Fixes: 6e18ed929d3b ("net: add helpers for setting a memory provider on an rx queue")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
- add to commit msg
- fix arg naming in the header (Stan)
v1: https://lore.kernel.org/20250331194303.2026903-1-kuba@kernel.org
CC: ap420073@gmail.com
CC: almasrymina@google.com
CC: asml.silence@gmail.com
CC: dw@davidwei.uk
CC: sdf@fomichev.me
---
include/net/page_pool/memory_provider.h | 6 +++
net/core/devmem.c | 52 ++++++-------------------
net/core/netdev-genl.c | 6 ---
net/core/netdev_rx_queue.c | 49 +++++++++++++++++------
4 files changed, 55 insertions(+), 58 deletions(-)
diff --git a/include/net/page_pool/memory_provider.h b/include/net/page_pool/memory_provider.h
index b3e665897767..ada4f968960a 100644
--- a/include/net/page_pool/memory_provider.h
+++ b/include/net/page_pool/memory_provider.h
@@ -6,6 +6,7 @@
#include <net/page_pool/types.h>
struct netdev_rx_queue;
+struct netlink_ext_ack;
struct sk_buff;
struct memory_provider_ops {
@@ -24,8 +25,13 @@ void net_mp_niov_clear_page_pool(struct net_iov *niov);
int net_mp_open_rxq(struct net_device *dev, unsigned ifq_idx,
struct pp_memory_provider_params *p);
+int __net_mp_open_rxq(struct net_device *dev, unsigned int rxq_idx,
+ const struct pp_memory_provider_params *p,
+ struct netlink_ext_ack *extack);
void net_mp_close_rxq(struct net_device *dev, unsigned ifq_idx,
struct pp_memory_provider_params *old_p);
+void __net_mp_close_rxq(struct net_device *dev, unsigned int rxq_idx,
+ const struct pp_memory_provider_params *old_p);
/**
* net_mp_netmem_place_in_cache() - give a netmem to a page pool
diff --git a/net/core/devmem.c b/net/core/devmem.c
index ee145a2aa41c..f2ce3c2ebc97 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -8,7 +8,6 @@
*/
#include <linux/dma-buf.h>
-#include <linux/ethtool_netlink.h>
#include <linux/genalloc.h>
#include <linux/mm.h>
#include <linux/netdevice.h>
@@ -143,57 +142,28 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
struct net_devmem_dmabuf_binding *binding,
struct netlink_ext_ack *extack)
{
+ struct pp_memory_provider_params mp_params = {
+ .mp_priv = binding,
+ .mp_ops = &dmabuf_devmem_ops,
+ };
struct netdev_rx_queue *rxq;
u32 xa_idx;
int err;
- if (rxq_idx >= dev->real_num_rx_queues) {
- NL_SET_ERR_MSG(extack, "rx queue index out of range");
- return -ERANGE;
- }
-
- if (dev->cfg->hds_config != ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
- NL_SET_ERR_MSG(extack, "tcp-data-split is disabled");
- return -EINVAL;
- }
-
- if (dev->cfg->hds_thresh) {
- NL_SET_ERR_MSG(extack, "hds-thresh is not zero");
- return -EINVAL;
- }
-
- rxq = __netif_get_rx_queue(dev, rxq_idx);
- if (rxq->mp_params.mp_ops) {
- NL_SET_ERR_MSG(extack, "designated queue already memory provider bound");
- return -EEXIST;
- }
-
-#ifdef CONFIG_XDP_SOCKETS
- if (rxq->pool) {
- NL_SET_ERR_MSG(extack, "designated queue already in use by AF_XDP");
- return -EBUSY;
- }
-#endif
-
- err = xa_alloc(&binding->bound_rxqs, &xa_idx, rxq, xa_limit_32b,
- GFP_KERNEL);
+ err = __net_mp_open_rxq(dev, rxq_idx, &mp_params, extack);
if (err)
return err;
- rxq->mp_params.mp_priv = binding;
- rxq->mp_params.mp_ops = &dmabuf_devmem_ops;
-
- err = netdev_rx_queue_restart(dev, rxq_idx);
+ rxq = __netif_get_rx_queue(dev, rxq_idx);
+ err = xa_alloc(&binding->bound_rxqs, &xa_idx, rxq, xa_limit_32b,
+ GFP_KERNEL);
if (err)
- goto err_xa_erase;
+ goto err_close_rxq;
return 0;
-err_xa_erase:
- rxq->mp_params.mp_priv = NULL;
- rxq->mp_params.mp_ops = NULL;
- xa_erase(&binding->bound_rxqs, xa_idx);
-
+err_close_rxq:
+ __net_mp_close_rxq(dev, rxq_idx, &mp_params);
return err;
}
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 3afeaa8c5dc5..5d7af50fe702 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -874,12 +874,6 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
goto err_unlock;
}
- if (dev_xdp_prog_count(netdev)) {
- NL_SET_ERR_MSG(info->extack, "unable to bind dmabuf to device with XDP program attached");
- err = -EEXIST;
- goto err_unlock;
- }
-
binding = net_devmem_bind_dmabuf(netdev, dmabuf_fd, info->extack);
if (IS_ERR(binding)) {
err = PTR_ERR(binding);
diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
index 3af716f77a13..556b5393ec9f 100644
--- a/net/core/netdev_rx_queue.c
+++ b/net/core/netdev_rx_queue.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/ethtool_netlink.h>
#include <linux/netdevice.h>
#include <net/netdev_lock.h>
#include <net/netdev_queues.h>
@@ -86,8 +87,9 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
}
EXPORT_SYMBOL_NS_GPL(netdev_rx_queue_restart, "NETDEV_INTERNAL");
-static int __net_mp_open_rxq(struct net_device *dev, unsigned ifq_idx,
- struct pp_memory_provider_params *p)
+int __net_mp_open_rxq(struct net_device *dev, unsigned int rxq_idx,
+ const struct pp_memory_provider_params *p,
+ struct netlink_ext_ack *extack)
{
struct netdev_rx_queue *rxq;
int ret;
@@ -95,16 +97,41 @@ static int __net_mp_open_rxq(struct net_device *dev, unsigned ifq_idx,
if (!netdev_need_ops_lock(dev))
return -EOPNOTSUPP;
- if (ifq_idx >= dev->real_num_rx_queues)
+ if (rxq_idx >= dev->real_num_rx_queues)
return -EINVAL;
- ifq_idx = array_index_nospec(ifq_idx, dev->real_num_rx_queues);
+ rxq_idx = array_index_nospec(rxq_idx, dev->real_num_rx_queues);
- rxq = __netif_get_rx_queue(dev, ifq_idx);
- if (rxq->mp_params.mp_ops)
+ if (rxq_idx >= dev->real_num_rx_queues) {
+ NL_SET_ERR_MSG(extack, "rx queue index out of range");
+ return -ERANGE;
+ }
+ if (dev->cfg->hds_config != ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
+ NL_SET_ERR_MSG(extack, "tcp-data-split is disabled");
+ return -EINVAL;
+ }
+ if (dev->cfg->hds_thresh) {
+ NL_SET_ERR_MSG(extack, "hds-thresh is not zero");
+ return -EINVAL;
+ }
+ if (dev_xdp_prog_count(dev)) {
+ NL_SET_ERR_MSG(extack, "unable to custom memory provider to device with XDP program attached");
return -EEXIST;
+ }
+
+ rxq = __netif_get_rx_queue(dev, rxq_idx);
+ if (rxq->mp_params.mp_ops) {
+ NL_SET_ERR_MSG(extack, "designated queue already memory provider bound");
+ return -EEXIST;
+ }
+#ifdef CONFIG_XDP_SOCKETS
+ if (rxq->pool) {
+ NL_SET_ERR_MSG(extack, "designated queue already in use by AF_XDP");
+ return -EBUSY;
+ }
+#endif
rxq->mp_params = *p;
- ret = netdev_rx_queue_restart(dev, ifq_idx);
+ ret = netdev_rx_queue_restart(dev, rxq_idx);
if (ret) {
rxq->mp_params.mp_ops = NULL;
rxq->mp_params.mp_priv = NULL;
@@ -112,19 +139,19 @@ static int __net_mp_open_rxq(struct net_device *dev, unsigned ifq_idx,
return ret;
}
-int net_mp_open_rxq(struct net_device *dev, unsigned ifq_idx,
+int net_mp_open_rxq(struct net_device *dev, unsigned int rxq_idx,
struct pp_memory_provider_params *p)
{
int ret;
netdev_lock(dev);
- ret = __net_mp_open_rxq(dev, ifq_idx, p);
+ ret = __net_mp_open_rxq(dev, rxq_idx, p, NULL);
netdev_unlock(dev);
return ret;
}
-static void __net_mp_close_rxq(struct net_device *dev, unsigned ifq_idx,
- struct pp_memory_provider_params *old_p)
+void __net_mp_close_rxq(struct net_device *dev, unsigned int ifq_idx,
+ const struct pp_memory_provider_params *old_p)
{
struct netdev_rx_queue *rxq;
--
2.49.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net v2 2/2] net: avoid false positive warnings in __net_mp_close_rxq()
2025-04-03 1:34 [PATCH net v2 0/2] net: make memory provider install / close paths more common Jakub Kicinski
2025-04-03 1:34 ` [PATCH net v2 1/2] net: move mp dev config validation to __net_mp_open_rxq() Jakub Kicinski
@ 2025-04-03 1:34 ` Jakub Kicinski
2025-04-03 21:37 ` Mina Almasry
2025-04-04 15:10 ` [PATCH net v2 0/2] net: make memory provider install / close paths more common patchwork-bot+netdevbpf
2 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2025-04-03 1:34 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, andrew+netdev, horms, ap420073,
almasrymina, asml.silence, dw, sdf, Jakub Kicinski
Commit under Fixes solved the problem of spurious warnings when we
uninstall an MP from a device while its down. The __net_mp_close_rxq()
which is used by io_uring was not fixed. Move the fix over and reuse
__net_mp_close_rxq() in the devmem path.
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Fixes: a70f891e0fa0 ("net: devmem: do not WARN conditionally after netdev_rx_queue_restart()")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
- don't split the registration check, it may cause a race if we just
bail on the registration state and not on the MP being present,
as we drop and re-take the instance lock after setting reg_state
v1: https://lore.kernel.org/20250331194308.2026940-1-kuba@kernel.org
---
net/core/devmem.c | 12 +++++-------
net/core/netdev_rx_queue.c | 4 +++-
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/net/core/devmem.c b/net/core/devmem.c
index f2ce3c2ebc97..6e27a47d0493 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -116,21 +116,19 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
struct netdev_rx_queue *rxq;
unsigned long xa_idx;
unsigned int rxq_idx;
- int err;
if (binding->list.next)
list_del(&binding->list);
xa_for_each(&binding->bound_rxqs, xa_idx, rxq) {
- WARN_ON(rxq->mp_params.mp_priv != binding);
-
- rxq->mp_params.mp_priv = NULL;
- rxq->mp_params.mp_ops = NULL;
+ const struct pp_memory_provider_params mp_params = {
+ .mp_priv = binding,
+ .mp_ops = &dmabuf_devmem_ops,
+ };
rxq_idx = get_netdev_rx_queue_index(rxq);
- err = netdev_rx_queue_restart(binding->dev, rxq_idx);
- WARN_ON(err && err != -ENETDOWN);
+ __net_mp_close_rxq(binding->dev, rxq_idx, &mp_params);
}
xa_erase(&net_devmem_dmabuf_bindings, binding->id);
diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
index 556b5393ec9f..d126f10197bf 100644
--- a/net/core/netdev_rx_queue.c
+++ b/net/core/netdev_rx_queue.c
@@ -154,6 +154,7 @@ void __net_mp_close_rxq(struct net_device *dev, unsigned int ifq_idx,
const struct pp_memory_provider_params *old_p)
{
struct netdev_rx_queue *rxq;
+ int err;
if (WARN_ON_ONCE(ifq_idx >= dev->real_num_rx_queues))
return;
@@ -173,7 +174,8 @@ void __net_mp_close_rxq(struct net_device *dev, unsigned int ifq_idx,
rxq->mp_params.mp_ops = NULL;
rxq->mp_params.mp_priv = NULL;
- WARN_ON(netdev_rx_queue_restart(dev, ifq_idx));
+ err = netdev_rx_queue_restart(dev, ifq_idx);
+ WARN_ON(err && err != -ENETDOWN);
}
void net_mp_close_rxq(struct net_device *dev, unsigned ifq_idx,
--
2.49.0
^ permalink raw reply related [flat|nested] 5+ messages in thread