[PATCH net-next] mctp: no longer rely on net->dev_index_head[]

[PATCH net-next] mctp: no longer rely on net->dev_index_head[]

[Eric Dumazet]

mctp_dump_addrinfo() is one of the last users of
net->dev_index_head[] in the control path.

Switch to for_each_netdev_dump() for better scalability.

Use C99 for mctp_device_rtnl_msg_handlers[] to prepare
future RTNL removal from mctp_dump_addrinfo()

(mdev->addrs is not yet RCU protected)

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jeremy Kerr <jk@codeconstruct.com.au>
Cc: Matt Johnston <matt@codeconstruct.com.au>
---
Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/mctp/device.c | 50 ++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/net/mctp/device.c b/net/mctp/device.c
index 26ce34b7e88e174cdb6fa65c0d8e5bf6b5a580d7..8e0724c56723de328592bfe5c6fc8085cd3102fe 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -20,8 +20,7 @@
 #include <net/sock.h>
 
 struct mctp_dump_cb {
-	int h;
-	int idx;
+	unsigned long ifindex;
 	size_t a_idx;
 };
 
@@ -115,43 +114,29 @@ static int mctp_dump_addrinfo(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct mctp_dump_cb *mcb = (void *)cb->ctx;
 	struct net *net = sock_net(skb->sk);
-	struct hlist_head *head;
 	struct net_device *dev;
 	struct ifaddrmsg *hdr;
 	struct mctp_dev *mdev;
-	int ifindex;
-	int idx = 0, rc;
+	int ifindex, rc;
 
 	hdr = nlmsg_data(cb->nlh);
 	// filter by ifindex if requested
 	ifindex = hdr->ifa_index;
 
 	rcu_read_lock();
-	for (; mcb->h < NETDEV_HASHENTRIES; mcb->h++, mcb->idx = 0) {
-		idx = 0;
-		head = &net->dev_index_head[mcb->h];
-		hlist_for_each_entry_rcu(dev, head, index_hlist) {
-			if (idx >= mcb->idx &&
-			    (ifindex == 0 || ifindex == dev->ifindex)) {
-				mdev = __mctp_dev_get(dev);
-				if (mdev) {
-					rc = mctp_dump_dev_addrinfo(mdev,
-								    skb, cb);
-					mctp_dev_put(mdev);
-					// Error indicates full buffer, this
-					// callback will get retried.
-					if (rc < 0)
-						goto out;
-				}
-			}
-			idx++;
-			// reset for next iteration
-			mcb->a_idx = 0;
-		}
+	for_each_netdev_dump(net, dev, mcb->ifindex) {
+		if (ifindex && ifindex != dev->ifindex)
+			continue;
+		mdev = __mctp_dev_get(dev);
+		if (!mdev)
+			continue;
+		rc = mctp_dump_dev_addrinfo(mdev, skb, cb);
+		mctp_dev_put(mdev);
+		if (rc < 0)
+			break;
+		mcb->a_idx = 0;
 	}
-out:
 	rcu_read_unlock();
-	mcb->idx = idx;
 
 	return skb->len;
 }
@@ -531,9 +516,12 @@ static struct notifier_block mctp_dev_nb = {
 };
 
 static const struct rtnl_msg_handler mctp_device_rtnl_msg_handlers[] = {
-	{THIS_MODULE, PF_MCTP, RTM_NEWADDR, mctp_rtm_newaddr, NULL, 0},
-	{THIS_MODULE, PF_MCTP, RTM_DELADDR, mctp_rtm_deladdr, NULL, 0},
-	{THIS_MODULE, PF_MCTP, RTM_GETADDR, NULL, mctp_dump_addrinfo, 0},
+	{.owner = THIS_MODULE, .protocol = PF_MCTP, .msgtype = RTM_NEWADDR,
+	 .doit = mctp_rtm_newaddr},
+	{.owner = THIS_MODULE, .protocol = PF_MCTP, .msgtype = RTM_DELADDR,
+	 .doit = mctp_rtm_deladdr},
+	{.owner = THIS_MODULE, .protocol = PF_MCTP, .msgtype = RTM_GETADDR,
+	 .dumpit = mctp_dump_addrinfo},
 };
 
 int __init mctp_device_init(void)
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH net-next] mctp: no longer rely on net->dev_index_head[]

[Kuniyuki Iwashima]

From: Eric Dumazet <edumazet@google.com>
Date: Fri,  6 Dec 2024 22:38:11 +0000
> mctp_dump_addrinfo() is one of the last users of
> net->dev_index_head[] in the control path.
> 
> Switch to for_each_netdev_dump() for better scalability.
> 
> Use C99 for mctp_device_rtnl_msg_handlers[] to prepare
> future RTNL removal from mctp_dump_addrinfo()
> 
> (mdev->addrs is not yet RCU protected)
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>


> Cc: Jeremy Kerr <jk@codeconstruct.com.au>
> Cc: Matt Johnston <matt@codeconstruct.com.au>
> ---
> Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>  net/mctp/device.c | 50 ++++++++++++++++++-----------------------------
>  1 file changed, 19 insertions(+), 31 deletions(-)
> 
> diff --git a/net/mctp/device.c b/net/mctp/device.c
> index 26ce34b7e88e174cdb6fa65c0d8e5bf6b5a580d7..8e0724c56723de328592bfe5c6fc8085cd3102fe 100644
> --- a/net/mctp/device.c
> +++ b/net/mctp/device.c
> @@ -20,8 +20,7 @@
>  #include <net/sock.h>
>  
>  struct mctp_dump_cb {
> -	int h;
> -	int idx;
> +	unsigned long ifindex;
>  	size_t a_idx;
>  };
>  
> @@ -115,43 +114,29 @@ static int mctp_dump_addrinfo(struct sk_buff *skb, struct netlink_callback *cb)
>  {
>  	struct mctp_dump_cb *mcb = (void *)cb->ctx;
>  	struct net *net = sock_net(skb->sk);
> -	struct hlist_head *head;
>  	struct net_device *dev;
>  	struct ifaddrmsg *hdr;
>  	struct mctp_dev *mdev;
> -	int ifindex;
> -	int idx = 0, rc;
> +	int ifindex, rc;
>  
>  	hdr = nlmsg_data(cb->nlh);
>  	// filter by ifindex if requested
>  	ifindex = hdr->ifa_index;
>  
>  	rcu_read_lock();
> -	for (; mcb->h < NETDEV_HASHENTRIES; mcb->h++, mcb->idx = 0) {
> -		idx = 0;
> -		head = &net->dev_index_head[mcb->h];
> -		hlist_for_each_entry_rcu(dev, head, index_hlist) {
> -			if (idx >= mcb->idx &&
> -			    (ifindex == 0 || ifindex == dev->ifindex)) {
> -				mdev = __mctp_dev_get(dev);
> -				if (mdev) {
> -					rc = mctp_dump_dev_addrinfo(mdev,
> -								    skb, cb);
> -					mctp_dev_put(mdev);
> -					// Error indicates full buffer, this
> -					// callback will get retried.
> -					if (rc < 0)
> -						goto out;
> -				}
> -			}
> -			idx++;
> -			// reset for next iteration
> -			mcb->a_idx = 0;
> -		}
> +	for_each_netdev_dump(net, dev, mcb->ifindex) {
> +		if (ifindex && ifindex != dev->ifindex)
> +			continue;
> +		mdev = __mctp_dev_get(dev);
> +		if (!mdev)
> +			continue;
> +		rc = mctp_dump_dev_addrinfo(mdev, skb, cb);
> +		mctp_dev_put(mdev);
> +		if (rc < 0)
> +			break;
> +		mcb->a_idx = 0;
>  	}
> -out:
>  	rcu_read_unlock();
> -	mcb->idx = idx;
>  
>  	return skb->len;
>  }
> @@ -531,9 +516,12 @@ static struct notifier_block mctp_dev_nb = {
>  };
>  
>  static const struct rtnl_msg_handler mctp_device_rtnl_msg_handlers[] = {
> -	{THIS_MODULE, PF_MCTP, RTM_NEWADDR, mctp_rtm_newaddr, NULL, 0},
> -	{THIS_MODULE, PF_MCTP, RTM_DELADDR, mctp_rtm_deladdr, NULL, 0},
> -	{THIS_MODULE, PF_MCTP, RTM_GETADDR, NULL, mctp_dump_addrinfo, 0},
> +	{.owner = THIS_MODULE, .protocol = PF_MCTP, .msgtype = RTM_NEWADDR,
> +	 .doit = mctp_rtm_newaddr},
> +	{.owner = THIS_MODULE, .protocol = PF_MCTP, .msgtype = RTM_DELADDR,
> +	 .doit = mctp_rtm_deladdr},
> +	{.owner = THIS_MODULE, .protocol = PF_MCTP, .msgtype = RTM_GETADDR,
> +	 .dumpit = mctp_dump_addrinfo},
>  };
>  
>  int __init mctp_device_init(void)
> -- 
> 2.47.0.338.g60cca15819-goog

Re: [PATCH net-next] mctp: no longer rely on net->dev_index_head[]

[Jeremy Kerr]

Hi Eric,

> mctp_dump_addrinfo() is one of the last users of
> net->dev_index_head[] in the control path.
> 
> Switch to for_each_netdev_dump() for better scalability.
> 
> Use C99 for mctp_device_rtnl_msg_handlers[] to prepare
> future RTNL removal from mctp_dump_addrinfo()

Nice one, thanks for this. All looks good testing the RTM_GETADDR dump
using the usual tools.

Acked-by: Jeremy Kerr <jk@codeconstruct.com.au>

Cheers,


Jeremy

Re: [PATCH net-next] mctp: no longer rely on net->dev_index_head[]

[patchwork-bot+netdevbpf]

Hello:

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

On Fri,  6 Dec 2024 22:38:11 +0000 you wrote:
> mctp_dump_addrinfo() is one of the last users of
> net->dev_index_head[] in the control path.
> 
> Switch to for_each_netdev_dump() for better scalability.
> 
> Use C99 for mctp_device_rtnl_msg_handlers[] to prepare
> future RTNL removal from mctp_dump_addrinfo()
> 
> [...]

Here is the summary with links:
  - [net-next] mctp: no longer rely on net->dev_index_head[]
    https://git.kernel.org/netdev/net-next/c/2d20773aec14

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