diff mbox series

[net,1/2] net: move mp dev config validation to __net_mp_open_rxq()

Message ID 20250331194303.2026903-1-kuba@kernel.org (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series net: make memory provider install / close paths more common | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 26 (+1) this patch: 26 (+1)
netdev/cc_maintainers warning 7 maintainers not CCed: daniel@iogearbox.net jdamato@fastly.com ast@kernel.org bpf@vger.kernel.org john.fastabend@gmail.com hawk@kernel.org ilias.apalodimas@linaro.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 5 this patch: 5
netdev/checkpatch warning WARNING: line length of 111 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2025-04-01--00-00 (tests: 902)

Commit Message

Jakub Kicinski March 31, 2025, 7:43 p.m. UTC 
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.

Fixes: 6e18ed929d3b ("net: add helpers for setting a memory provider on an rx queue")
Signed-off-by: Jakub Kicinski <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(-)

Comments

Stanislav Fomichev March 31, 2025, 8:39 p.m. UTC | #1 
On 03/31, Jakub Kicinski wrote:
> 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.
> 
> Fixes: 6e18ed929d3b ("net: add helpers for setting a memory provider on an rx queue")
> Signed-off-by: Jakub Kicinski <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..3724ac3c9fb2 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 ifq_idx,
> +		      const struct pp_memory_provider_params *p,
> +		      struct netlink_ext_ack *extack);

Nit: you keep using ifq_idx here, but in the .c file you rename it to
rxq_idx. Not worth the respin..

Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Pavel Begunkov April 1, 2025, 11:37 a.m. UTC | #2 
On 3/31/25 20:43, Jakub Kicinski wrote:
> 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.

Looks good, one question below

...
> 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 @@
...
> -
> -	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;

Was reversing the order b/w open and xa_alloc intentional?
It didn't need __net_mp_close_rxq() before, which is a good thing
considering the error handling in __net_mp_close_rxq is a bit
flaky (i.e. the WARN_ON at the end).

>   
> -	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;
>   }
Jakub Kicinski April 1, 2025, 3 p.m. UTC | #3 
On Tue, 1 Apr 2025 12:37:34 +0100 Pavel Begunkov wrote:
> > -	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;  
> 
> Was reversing the order b/w open and xa_alloc intentional?
> It didn't need __net_mp_close_rxq() before, which is a good thing
> considering the error handling in __net_mp_close_rxq is a bit
> flaky (i.e. the WARN_ON at the end).

Should have mentioned.. yes, intentional, otherwise we'd either have to
insert a potentially invalid rxq pointer into the xarray or duplicate
the rxq bounds check. Inserting invalid pointer and deleting it immediately
should be okay, since readers take the instance lock, but felt a little
dirty. In practice xa_alloc() failures should be extremely rare here so
I went with the reorder.
diff mbox series

Patch

diff --git a/include/net/page_pool/memory_provider.h b/include/net/page_pool/memory_provider.h
index b3e665897767..3724ac3c9fb2 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 ifq_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 ifq_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 fd1cfa9707dc..8ad4a944f368 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;