[PATCH net-next 0/2] rtnetlink: rtnl_stats_dump() changes

[PATCH net-next 0/2] rtnetlink: rtnl_stats_dump() changes

[Eric Dumazet]

Getting rid of RTNL in rtnl_stats_dump() looks challenging.

In the meantime, we can:

1) Avoid RTNL acquisition for the final NLMSG_DONE marker.

2) Use for_each_netdev_dump() instead of the net->dev_index_head[]
   hash table.

Eric Dumazet (2):
  rtnetlink: change rtnl_stats_dump() return value
  rtnetlink: use for_each_netdev_dump() in rtnl_stats_dump()

 net/core/rtnetlink.c | 61 +++++++++++++++++---------------------------
 1 file changed, 23 insertions(+), 38 deletions(-)

-- 
2.45.0.rc0.197.gbae5840b3b-goog

[PATCH net-next 1/2] rtnetlink: change rtnl_stats_dump() return value

[Eric Dumazet]

By returning 0 (or an error) instead of skb->len,
we allow NLMSG_DONE to be appended to the current
skb at the end of a dump, saving a couple of recvmsg()
system calls.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 283e42f48af68504af193ed5763d4e0fcd667d99..88980c8bcf334079e2d19cbcfb3f10fc05e3c19b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -6024,7 +6024,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	cb->args[1] = idx;
 	cb->args[0] = h;
 
-	return skb->len;
+	return err;
 }
 
 void rtnl_offload_xstats_notify(struct net_device *dev)
-- 
2.45.0.rc0.197.gbae5840b3b-goog

[PATCH net-next 2/2] rtnetlink: use for_each_netdev_dump() in rtnl_stats_dump()

[Eric Dumazet]

Switch rtnl_stats_dump() to use for_each_netdev_dump()
instead of net->dev_index_head[] hash table.

This makes the code much easier to read, and fixes
scalability issues.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/rtnetlink.c | 59 +++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 37 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 88980c8bcf334079e2d19cbcfb3f10fc05e3c19b..28050e53ecb025f8d207f4b38805fb108799ca65 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -5961,19 +5961,17 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh,
 static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct netlink_ext_ack *extack = cb->extack;
-	int h, s_h, err, s_idx, s_idxattr, s_prividx;
 	struct rtnl_stats_dump_filters filters;
 	struct net *net = sock_net(skb->sk);
 	unsigned int flags = NLM_F_MULTI;
 	struct if_stats_msg *ifsm;
-	struct hlist_head *head;
+	struct {
+		unsigned long ifindex;
+		int idxattr;
+		int prividx;
+	} *ctx = (void *)cb->ctx;
 	struct net_device *dev;
-	int idx = 0;
-
-	s_h = cb->args[0];
-	s_idx = cb->args[1];
-	s_idxattr = cb->args[2];
-	s_prividx = cb->args[3];
+	int err;
 
 	cb->seq = net->dev_base_seq;
 
@@ -5992,37 +5990,24 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	if (err)
 		return err;
 
-	for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
-		idx = 0;
-		head = &net->dev_index_head[h];
-		hlist_for_each_entry(dev, head, index_hlist) {
-			if (idx < s_idx)
-				goto cont;
-			err = rtnl_fill_statsinfo(skb, dev, RTM_NEWSTATS,
-						  NETLINK_CB(cb->skb).portid,
-						  cb->nlh->nlmsg_seq, 0,
-						  flags, &filters,
-						  &s_idxattr, &s_prividx,
-						  extack);
-			/* If we ran out of room on the first message,
-			 * we're in trouble
-			 */
-			WARN_ON((err == -EMSGSIZE) && (skb->len == 0));
+	for_each_netdev_dump(net, dev, ctx->ifindex) {
+		err = rtnl_fill_statsinfo(skb, dev, RTM_NEWSTATS,
+					  NETLINK_CB(cb->skb).portid,
+					  cb->nlh->nlmsg_seq, 0,
+					  flags, &filters,
+					  &ctx->idxattr, &ctx->prividx,
+					  extack);
+		/* If we ran out of room on the first message,
+		 * we're in trouble.
+		 */
+		WARN_ON((err == -EMSGSIZE) && (skb->len == 0));
 
-			if (err < 0)
-				goto out;
-			s_prividx = 0;
-			s_idxattr = 0;
-			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
-cont:
-			idx++;
-		}
+		if (err < 0)
+			break;
+		ctx->prividx = 0;
+		ctx->idxattr = 0;
+		nl_dump_check_consistent(cb, nlmsg_hdr(skb));
 	}
-out:
-	cb->args[3] = s_prividx;
-	cb->args[2] = s_idxattr;
-	cb->args[1] = idx;
-	cb->args[0] = h;
 
 	return err;
 }
-- 
2.45.0.rc0.197.gbae5840b3b-goog

Re: [PATCH net-next 1/2] rtnetlink: change rtnl_stats_dump() return value

[David Ahern]

On 5/2/24 5:37 AM, Eric Dumazet wrote:
> By returning 0 (or an error) instead of skb->len,
> we allow NLMSG_DONE to be appended to the current
> skb at the end of a dump, saving a couple of recvmsg()
> system calls.

any concern that a patch similar to:
https://lore.kernel.org/netdev/20240411180202.399246-1-kuba@kernel.org/
will be needed again here?

> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/core/rtnetlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 283e42f48af68504af193ed5763d4e0fcd667d99..88980c8bcf334079e2d19cbcfb3f10fc05e3c19b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -6024,7 +6024,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	cb->args[1] = idx;
>  	cb->args[0] = h;
>  
> -	return skb->len;
> +	return err;
>  }
>  
>  void rtnl_offload_xstats_notify(struct net_device *dev)


Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 2/2] rtnetlink: use for_each_netdev_dump() in rtnl_stats_dump()

[David Ahern]

On 5/2/24 5:37 AM, Eric Dumazet wrote:
> Switch rtnl_stats_dump() to use for_each_netdev_dump()
> instead of net->dev_index_head[] hash table.
> 
> This makes the code much easier to read, and fixes
> scalability issues.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/core/rtnetlink.c | 59 +++++++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 37 deletions(-)
> 


Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 1/2] rtnetlink: change rtnl_stats_dump() return value

[Eric Dumazet]

On Thu, May 2, 2024 at 5:59 PM David Ahern <dsahern@kernel.org> wrote:
>
> On 5/2/24 5:37 AM, Eric Dumazet wrote:
> > By returning 0 (or an error) instead of skb->len,
> > we allow NLMSG_DONE to be appended to the current
> > skb at the end of a dump, saving a couple of recvmsg()
> > system calls.
>
> any concern that a patch similar to:
> https://lore.kernel.org/netdev/20240411180202.399246-1-kuba@kernel.org/
> will be needed again here?

This has been discussed, Jakub answer was :

https://lore.kernel.org/netdev/20240411115748.05faa636@kernel.org/

So the plan is to change functions until a regression is reported.

Re: [PATCH net-next 0/2] rtnetlink: rtnl_stats_dump() changes

[patchwork-bot+netdevbpf]

Hello:

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

On Thu,  2 May 2024 11:37:46 +0000 you wrote:
> Getting rid of RTNL in rtnl_stats_dump() looks challenging.
> 
> In the meantime, we can:
> 
> 1) Avoid RTNL acquisition for the final NLMSG_DONE marker.
> 
> 2) Use for_each_netdev_dump() instead of the net->dev_index_head[]
>    hash table.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] rtnetlink: change rtnl_stats_dump() return value
    https://git.kernel.org/netdev/net-next/c/136c2a9a2a87
  - [net-next,2/2] rtnetlink: use for_each_netdev_dump() in rtnl_stats_dump()
    https://git.kernel.org/netdev/net-next/c/0feb396f7428

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

Re: [PATCH net-next 1/2] rtnetlink: change rtnl_stats_dump() return value

[David Ahern]

On 5/2/24 10:03 AM, Eric Dumazet wrote:
> On Thu, May 2, 2024 at 5:59 PM David Ahern <dsahern@kernel.org> wrote:
>>
>> On 5/2/24 5:37 AM, Eric Dumazet wrote:
>>> By returning 0 (or an error) instead of skb->len,
>>> we allow NLMSG_DONE to be appended to the current
>>> skb at the end of a dump, saving a couple of recvmsg()
>>> system calls.
>>
>> any concern that a patch similar to:
>> https://lore.kernel.org/netdev/20240411180202.399246-1-kuba@kernel.org/
>> will be needed again here?
> 
> This has been discussed, Jakub answer was :
> 
> https://lore.kernel.org/netdev/20240411115748.05faa636@kernel.org/
> 
> So the plan is to change functions until a regression is reported.
> 

As I commented in the past, it is more user friendly to add such
comments to a commit message so that when a regression occurs and a
bisect is done, the user hitting the regression sees the problem with an
obvious resolution.

Re: [PATCH net-next 1/2] rtnetlink: change rtnl_stats_dump() return value

[Eric Dumazet]

On Mon, May 6, 2024 at 6:23 PM David Ahern <dsahern@kernel.org> wrote:
>
> On 5/2/24 10:03 AM, Eric Dumazet wrote:
> > On Thu, May 2, 2024 at 5:59 PM David Ahern <dsahern@kernel.org> wrote:
> >>
> >> On 5/2/24 5:37 AM, Eric Dumazet wrote:
> >>> By returning 0 (or an error) instead of skb->len,
> >>> we allow NLMSG_DONE to be appended to the current
> >>> skb at the end of a dump, saving a couple of recvmsg()
> >>> system calls.
> >>
> >> any concern that a patch similar to:
> >> https://lore.kernel.org/netdev/20240411180202.399246-1-kuba@kernel.org/
> >> will be needed again here?
> >
> > This has been discussed, Jakub answer was :
> >
> > https://lore.kernel.org/netdev/20240411115748.05faa636@kernel.org/
> >
> > So the plan is to change functions until a regression is reported.
> >
>
> As I commented in the past, it is more user friendly to add such
> comments to a commit message so that when a regression occurs and a
> bisect is done, the user hitting the regression sees the problem with an
> obvious resolution.

This commit has a single line being changed.

Whoever does a bisection will report, and the resolution is trivial.

I do not think we want to copy/paste a full page of all links to all
relevant commits.