diff mbox series

[v2,net-next,03/14] ipv6: prepare inet6_fill_ifinfo() for RCU protection

Message ID 20240222105021.1943116-4-edumazet@google.com (mailing list archive)
State Accepted
Commit 8afc7a78d55de726b2747d7775c54def79509ec5
Delegated to: Netdev Maintainers
Headers show
Series rtnetlink: reduce RTNL pressure for dumps | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5047 this patch: 5047
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: dsahern@kernel.org
netdev/build_clang success Errors and warnings before: 1076 this patch: 1076
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 No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 5349 this patch: 5349
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 55 lines checked
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-2024-02-23--03-00 (tests: 1457)

Commit Message

Eric Dumazet Feb. 22, 2024, 10:50 a.m. UTC 
We want to use RCU protection instead of RTNL
for inet6_fill_ifinfo().

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/netdevice.h |  6 ++++--
 net/core/dev.c            |  4 ++--
 net/ipv6/addrconf.c       | 11 +++++++----
 3 files changed, 13 insertions(+), 8 deletions(-)

Comments

Jiri Pirko Feb. 22, 2024, 4:36 p.m. UTC | #1 
Thu, Feb 22, 2024 at 11:50:10AM CET, edumazet@google.com wrote:
>We want to use RCU protection instead of RTNL

Is this a royal "We"? :)


>for inet6_fill_ifinfo().

This is a motivation for this patch, not what the patch does.

Would it be possible to maintain some sort of culture for the patch
descriptions, even of the patches which are small and simple?

https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes

Your patch descriptions are usually hard to follow for me to understand
what the patch does :( Yes, I know you do it "to displease me" as you
wrote couple of months ago but maybe think about the others too, also
the ones looking in a git log/show and guessing.

Don't beat me.


>
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>---
> include/linux/netdevice.h |  6 ++++--
> net/core/dev.c            |  4 ++--
> net/ipv6/addrconf.c       | 11 +++++++----
> 3 files changed, 13 insertions(+), 8 deletions(-)
>
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index f07c8374f29cb936fe11236fc63e06e741b1c965..09023e44db4e2c3a2133afc52ba5a335d6030646 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -4354,8 +4354,10 @@ static inline bool netif_testing(const struct net_device *dev)
>  */
> static inline bool netif_oper_up(const struct net_device *dev)
> {
>-	return (dev->operstate == IF_OPER_UP ||
>-		dev->operstate == IF_OPER_UNKNOWN /* backward compat */);
>+	unsigned int operstate = READ_ONCE(dev->operstate);
>+
>+	return	operstate == IF_OPER_UP ||

double space  ^^


>+		operstate == IF_OPER_UNKNOWN /* backward compat */;
> }
> 
> /**
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 0628d8ff1ed932efdd45ab7b79599dcfcca6c4eb..275fd5259a4a92d0bd2e145d66a716248b6c2804 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -8632,12 +8632,12 @@ unsigned int dev_get_flags(const struct net_device *dev)
> {
> 	unsigned int flags;
> 
>-	flags = (dev->flags & ~(IFF_PROMISC |
>+	flags = (READ_ONCE(dev->flags) & ~(IFF_PROMISC |
> 				IFF_ALLMULTI |
> 				IFF_RUNNING |
> 				IFF_LOWER_UP |
> 				IFF_DORMANT)) |
>-		(dev->gflags & (IFF_PROMISC |
>+		(READ_ONCE(dev->gflags) & (IFF_PROMISC |
> 				IFF_ALLMULTI));
> 
> 	if (netif_running(dev)) {
>diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>index 3c8bdad0105dc9542489b612890ba86de9c44bdc..df3c6feea74e2d95144140eceb6df5cef2dce1f4 100644
>--- a/net/ipv6/addrconf.c
>+++ b/net/ipv6/addrconf.c
>@@ -6047,6 +6047,7 @@ static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,
> 	struct net_device *dev = idev->dev;
> 	struct ifinfomsg *hdr;
> 	struct nlmsghdr *nlh;
>+	int ifindex, iflink;
> 	void *protoinfo;
> 
> 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(*hdr), flags);
>@@ -6057,16 +6058,18 @@ static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,
> 	hdr->ifi_family = AF_INET6;
> 	hdr->__ifi_pad = 0;
> 	hdr->ifi_type = dev->type;
>-	hdr->ifi_index = dev->ifindex;
>+	ifindex = READ_ONCE(dev->ifindex);
>+	hdr->ifi_index = ifindex;
> 	hdr->ifi_flags = dev_get_flags(dev);
> 	hdr->ifi_change = 0;
> 
>+	iflink = dev_get_iflink(dev);
> 	if (nla_put_string(skb, IFLA_IFNAME, dev->name) ||
> 	    (dev->addr_len &&
> 	     nla_put(skb, IFLA_ADDRESS, dev->addr_len, dev->dev_addr)) ||
>-	    nla_put_u32(skb, IFLA_MTU, dev->mtu) ||
>-	    (dev->ifindex != dev_get_iflink(dev) &&
>-	     nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev))) ||
>+	    nla_put_u32(skb, IFLA_MTU, READ_ONCE(dev->mtu)) ||
>+	    (ifindex != iflink &&
>+	     nla_put_u32(skb, IFLA_LINK, iflink)) ||
> 	    nla_put_u8(skb, IFLA_OPERSTATE,
> 		       netif_running(dev) ? READ_ONCE(dev->operstate) : IF_OPER_DOWN))
> 		goto nla_put_failure;
>-- 
>2.44.0.rc1.240.g4c46232300-goog
>
>
Eric Dumazet Feb. 22, 2024, 4:43 p.m. UTC | #2 
On Thu, Feb 22, 2024 at 5:36 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Thu, Feb 22, 2024 at 11:50:10AM CET, edumazet@google.com wrote:
> >We want to use RCU protection instead of RTNL
>
> Is this a royal "We"? :)
>
>
> >for inet6_fill_ifinfo().
>
> This is a motivation for this patch, not what the patch does.
>
> Would it be possible to maintain some sort of culture for the patch
> descriptions, even of the patches which are small and simple?
>
> https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes
>
> Your patch descriptions are usually hard to follow for me to understand
> what the patch does :( Yes, I know you do it "to displease me" as you
> wrote couple of months ago but maybe think about the others too, also
> the ones looking in a git log/show and guessing.
>
> Don't beat me.
>

I dunno.

Do I need to explain why we need READ_ONCE()/WRITE_ONCE() on RCU for
all the patches ?

Documentation/RCU has already 36000 lines...
Eric Dumazet Feb. 22, 2024, 4:45 p.m. UTC | #3 
On Thu, Feb 22, 2024 at 5:36 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Thu, Feb 22, 2024 at 11:50:10AM CET, edumazet@google.com wrote:
> >We want to use RCU protection instead of RTNL
>
> Is this a royal "We"? :)

I was hoping reducing RTNL pressure was a team effort.

If not, maybe I should consider doing something else, if hundreds of
kernel engineers are adding more and more stuff depending on RTNL.
Jiri Pirko Feb. 23, 2024, 7:16 a.m. UTC | #4 
Thu, Feb 22, 2024 at 05:45:20PM CET, edumazet@google.com wrote:
>On Thu, Feb 22, 2024 at 5:36 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Feb 22, 2024 at 11:50:10AM CET, edumazet@google.com wrote:
>> >We want to use RCU protection instead of RTNL
>>
>> Is this a royal "We"? :)
>
>I was hoping reducing RTNL pressure was a team effort.

Yeah sure, it just reads odd to me, that's it. Basically if you state
the motivation in the cover letter, then in the patches you just tell
the codebase what to do and this "we want" statement become redundant.


>
>If not, maybe I should consider doing something else, if hundreds of
>kernel engineers are adding more and more stuff depending on RTNL.
Jiri Pirko Feb. 23, 2024, 7:19 a.m. UTC | #5 
Thu, Feb 22, 2024 at 05:43:17PM CET, edumazet@google.com wrote:
>On Thu, Feb 22, 2024 at 5:36 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Feb 22, 2024 at 11:50:10AM CET, edumazet@google.com wrote:
>> >We want to use RCU protection instead of RTNL
>>
>> Is this a royal "We"? :)
>>
>>
>> >for inet6_fill_ifinfo().
>>
>> This is a motivation for this patch, not what the patch does.
>>
>> Would it be possible to maintain some sort of culture for the patch
>> descriptions, even of the patches which are small and simple?
>>
>> https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes
>>
>> Your patch descriptions are usually hard to follow for me to understand
>> what the patch does :( Yes, I know you do it "to displease me" as you
>> wrote couple of months ago but maybe think about the others too, also
>> the ones looking in a git log/show and guessing.
>>
>> Don't beat me.
>>
>
>I dunno.
>
>Do I need to explain why we need READ_ONCE()/WRITE_ONCE() on RCU for
>all the patches ?

I don't think so. If the motivation is described in the cover letter
properly, then in the incremental patches you just tell the codebase
what to change clearly, that describes the matter of changes. No
redundancy, clear motivation, clear patch description, easy to
understand for everyone.


>
>Documentation/RCU has already 36000 lines...
diff mbox series

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f07c8374f29cb936fe11236fc63e06e741b1c965..09023e44db4e2c3a2133afc52ba5a335d6030646 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4354,8 +4354,10 @@  static inline bool netif_testing(const struct net_device *dev)
  */
 static inline bool netif_oper_up(const struct net_device *dev)
 {
-	return (dev->operstate == IF_OPER_UP ||
-		dev->operstate == IF_OPER_UNKNOWN /* backward compat */);
+	unsigned int operstate = READ_ONCE(dev->operstate);
+
+	return	operstate == IF_OPER_UP ||
+		operstate == IF_OPER_UNKNOWN /* backward compat */;
 }
 
 /**
diff --git a/net/core/dev.c b/net/core/dev.c
index 0628d8ff1ed932efdd45ab7b79599dcfcca6c4eb..275fd5259a4a92d0bd2e145d66a716248b6c2804 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8632,12 +8632,12 @@  unsigned int dev_get_flags(const struct net_device *dev)
 {
 	unsigned int flags;
 
-	flags = (dev->flags & ~(IFF_PROMISC |
+	flags = (READ_ONCE(dev->flags) & ~(IFF_PROMISC |
 				IFF_ALLMULTI |
 				IFF_RUNNING |
 				IFF_LOWER_UP |
 				IFF_DORMANT)) |
-		(dev->gflags & (IFF_PROMISC |
+		(READ_ONCE(dev->gflags) & (IFF_PROMISC |
 				IFF_ALLMULTI));
 
 	if (netif_running(dev)) {
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 3c8bdad0105dc9542489b612890ba86de9c44bdc..df3c6feea74e2d95144140eceb6df5cef2dce1f4 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -6047,6 +6047,7 @@  static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,
 	struct net_device *dev = idev->dev;
 	struct ifinfomsg *hdr;
 	struct nlmsghdr *nlh;
+	int ifindex, iflink;
 	void *protoinfo;
 
 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(*hdr), flags);
@@ -6057,16 +6058,18 @@  static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,
 	hdr->ifi_family = AF_INET6;
 	hdr->__ifi_pad = 0;
 	hdr->ifi_type = dev->type;
-	hdr->ifi_index = dev->ifindex;
+	ifindex = READ_ONCE(dev->ifindex);
+	hdr->ifi_index = ifindex;
 	hdr->ifi_flags = dev_get_flags(dev);
 	hdr->ifi_change = 0;
 
+	iflink = dev_get_iflink(dev);
 	if (nla_put_string(skb, IFLA_IFNAME, dev->name) ||
 	    (dev->addr_len &&
 	     nla_put(skb, IFLA_ADDRESS, dev->addr_len, dev->dev_addr)) ||
-	    nla_put_u32(skb, IFLA_MTU, dev->mtu) ||
-	    (dev->ifindex != dev_get_iflink(dev) &&
-	     nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev))) ||
+	    nla_put_u32(skb, IFLA_MTU, READ_ONCE(dev->mtu)) ||
+	    (ifindex != iflink &&
+	     nla_put_u32(skb, IFLA_LINK, iflink)) ||
 	    nla_put_u8(skb, IFLA_OPERSTATE,
 		       netif_running(dev) ? READ_ONCE(dev->operstate) : IF_OPER_DOWN))
 		goto nla_put_failure;