| Message ID | 20210425155742.30057-2-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fix lockdep false positive splat | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 6930 this patch: 6930 |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 1 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 114 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 7144 this patch: 7144 |
| netdev/header_inline | success | Link |
On 25.04.2021 17:57, Taehee Yoo wrote: > The purpose of bond_get_lowest_level_rcu() is to get nested_level under > RCU. Because dev->nested_level is protected by RTNL, so it shouldn't be > used without RTNL. But bonding module needs this value under RCU without > RTNL. > So, this function was added. > > But, there is another module, which needs this function. > So, make this function generic. > the new name is netdev_get_nest_level_rcu(). > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > > v2: > - No change > > drivers/net/bonding/bond_main.c | 45 +-------------------------------- > include/linux/netdevice.h | 1 + > net/core/dev.c | 44 ++++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+), 44 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 83ef62db6285..a9feb039ffa6 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -3754,47 +3754,6 @@ static void bond_fold_stats(struct rtnl_link_stats64 *_res, > } > } > > -#ifdef CONFIG_LOCKDEP > -static int bond_get_lowest_level_rcu(struct net_device *dev) > -{ > - struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1]; > - struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1]; > - int cur = 0, max = 0; > - > - now = dev; > - iter = &dev->adj_list.lower; > - > - while (1) { > - next = NULL; > - while (1) { > - ldev = netdev_next_lower_dev_rcu(now, &iter); > - if (!ldev) > - break; > - > - next = ldev; > - niter = &ldev->adj_list.lower; > - dev_stack[cur] = now; > - iter_stack[cur++] = iter; > - if (max <= cur) > - max = cur; > - break; > - } > - > - if (!next) { > - if (!cur) > - return max; > - next = dev_stack[--cur]; > - niter = iter_stack[cur]; > - } > - > - now = next; > - iter = niter; > - } > - > - return max; > -} > -#endif > - > static void bond_get_stats(struct net_device *bond_dev, > struct rtnl_link_stats64 *stats) > { > @@ -3806,9 +3765,7 @@ static void bond_get_stats(struct net_device *bond_dev, > > > rcu_read_lock(); > -#ifdef CONFIG_LOCKDEP > - nest_level = bond_get_lowest_level_rcu(bond_dev); > -#endif > + nest_level = netdev_get_nest_level_rcu(bond_dev); > > spin_lock_nested(&bond->stats_lock, nest_level); > memcpy(stats, &bond->bond_stats, sizeof(*stats)); > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 87a5d186faff..507c06bf5dba 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -4699,6 +4699,7 @@ int netdev_walk_all_lower_dev(struct net_device *dev, > int (*fn)(struct net_device *lower_dev, > struct netdev_nested_priv *priv), > struct netdev_nested_priv *priv); > +int netdev_get_nest_level_rcu(struct net_device *dev); > int netdev_walk_all_lower_dev_rcu(struct net_device *dev, > int (*fn)(struct net_device *lower_dev, > struct netdev_nested_priv *priv), > diff --git a/net/core/dev.c b/net/core/dev.c > index 15fe36332fb8..efc2bf88eafd 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -7709,6 +7709,50 @@ static int __netdev_update_lower_level(struct net_device *dev, > return 0; > } > > +int netdev_get_nest_level_rcu(struct net_device *dev) > +{ > +#ifdef CONFIG_LOCKDEP > + struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1]; > + struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1]; > + int cur = 0, max = 0; > + > + now = dev; > + iter = &dev->adj_list.lower; > + > + while (1) { > + next = NULL; > + while (1) { > + ldev = netdev_next_lower_dev_rcu(now, &iter); > + if (!ldev) > + break; > + > + next = ldev; > + niter = &ldev->adj_list.lower; > + dev_stack[cur] = now; > + iter_stack[cur++] = iter; > + if (max <= cur) > + max = cur; > + break; This looks odd. Why a while loop if it's left in the first iteration anyway? The whole loop looks unnecessarily complex. The following should do the same, just in a simpler way (untested!) while (1) { ldev = netdev_next_lower_dev_rcu(now, &iter); if (ldev) { dev_stack[cur] = now; iter_stack[cur++] = iter; if (max <= cur) max = cur; now = ldev; iter = &ldev->adj_list.lower; } else { if (!cur) break; now = dev_stack[--cur]; iter = iter_stack[cur]; } } I know that you just copied the original function. Simplifying the function should be something for a follow-up patch. > + } > + > + if (!next) { > + if (!cur) > + return max; > + next = dev_stack[--cur]; > + niter = iter_stack[cur]; > + } > + > + now = next; > + iter = niter; > + } > + > + return max; > +#else > + return 0; > +#endif > +} > +EXPORT_SYMBOL_GPL(netdev_get_nest_level_rcu); > + > int netdev_walk_all_lower_dev_rcu(struct net_device *dev, > int (*fn)(struct net_device *dev, > struct netdev_nested_priv *priv), >
On 4/26/21 3:03 AM, Heiner Kallweit wrote: Hi Heiner, Thank you for the review! > On 25.04.2021 17:57, Taehee Yoo wrote: >> The purpose of bond_get_lowest_level_rcu() is to get nested_level under >> RCU. Because dev->nested_level is protected by RTNL, so it shouldn't be >> used without RTNL. But bonding module needs this value under RCU without >> RTNL. >> So, this function was added. >> >> But, there is another module, which needs this function. >> So, make this function generic. >> the new name is netdev_get_nest_level_rcu(). >> >> Signed-off-by: Taehee Yoo <ap420073@gmail.com> >> --- >> >> v2: >> - No change >> >> drivers/net/bonding/bond_main.c | 45 +-------------------------------- >> include/linux/netdevice.h | 1 + >> net/core/dev.c | 44 ++++++++++++++++++++++++++++++++ >> 3 files changed, 46 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 83ef62db6285..a9feb039ffa6 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -3754,47 +3754,6 @@ static void bond_fold_stats(struct rtnl_link_stats64 *_res, >> } >> } >> >> -#ifdef CONFIG_LOCKDEP >> -static int bond_get_lowest_level_rcu(struct net_device *dev) >> -{ >> - struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1]; >> - struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1]; >> - int cur = 0, max = 0; >> - >> - now = dev; >> - iter = &dev->adj_list.lower; >> - >> - while (1) { >> - next = NULL; >> - while (1) { >> - ldev = netdev_next_lower_dev_rcu(now, &iter); >> - if (!ldev) >> - break; >> - >> - next = ldev; >> - niter = &ldev->adj_list.lower; >> - dev_stack[cur] = now; >> - iter_stack[cur++] = iter; >> - if (max <= cur) >> - max = cur; >> - break; >> - } >> - >> - if (!next) { >> - if (!cur) >> - return max; >> - next = dev_stack[--cur]; >> - niter = iter_stack[cur]; >> - } >> - >> - now = next; >> - iter = niter; >> - } >> - >> - return max; >> -} >> -#endif >> - >> static void bond_get_stats(struct net_device *bond_dev, >> struct rtnl_link_stats64 *stats) >> { >> @@ -3806,9 +3765,7 @@ static void bond_get_stats(struct net_device *bond_dev, >> >> >> rcu_read_lock(); >> -#ifdef CONFIG_LOCKDEP >> - nest_level = bond_get_lowest_level_rcu(bond_dev); >> -#endif >> + nest_level = netdev_get_nest_level_rcu(bond_dev); >> >> spin_lock_nested(&bond->stats_lock, nest_level); >> memcpy(stats, &bond->bond_stats, sizeof(*stats)); >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 87a5d186faff..507c06bf5dba 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -4699,6 +4699,7 @@ int netdev_walk_all_lower_dev(struct net_device *dev, >> int (*fn)(struct net_device *lower_dev, >> struct netdev_nested_priv *priv), >> struct netdev_nested_priv *priv); >> +int netdev_get_nest_level_rcu(struct net_device *dev); >> int netdev_walk_all_lower_dev_rcu(struct net_device *dev, >> int (*fn)(struct net_device *lower_dev, >> struct netdev_nested_priv *priv), >> diff --git a/net/core/dev.c b/net/core/dev.c >> index 15fe36332fb8..efc2bf88eafd 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -7709,6 +7709,50 @@ static int __netdev_update_lower_level(struct net_device *dev, >> return 0; >> } >> >> +int netdev_get_nest_level_rcu(struct net_device *dev) >> +{ >> +#ifdef CONFIG_LOCKDEP >> + struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1]; >> + struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1]; >> + int cur = 0, max = 0; >> + >> + now = dev; >> + iter = &dev->adj_list.lower; >> + >> + while (1) { >> + next = NULL; >> + while (1) { >> + ldev = netdev_next_lower_dev_rcu(now, &iter); >> + if (!ldev) >> + break; >> + >> + next = ldev; >> + niter = &ldev->adj_list.lower; >> + dev_stack[cur] = now; >> + iter_stack[cur++] = iter; >> + if (max <= cur) >> + max = cur; >> + break; > > This looks odd. Why a while loop if it's left in the first iteration > anyway? The whole loop looks unnecessarily complex. The following > should do the same, just in a simpler way (untested!) > > while (1) { > ldev = netdev_next_lower_dev_rcu(now, &iter); > if (ldev) { > dev_stack[cur] = now; > iter_stack[cur++] = iter; > if (max <= cur) > max = cur; > now = ldev; > iter = &ldev->adj_list.lower; > } else { > if (!cur) > break; > now = dev_stack[--cur]; > iter = iter_stack[cur]; > } > } > > I know that you just copied the original function. > Simplifying the function should be something for a > follow-up patch. > >> + } >> + >> + if (!next) { >> + if (!cur) >> + return max; >> + next = dev_stack[--cur]; >> + niter = iter_stack[cur]; >> + } >> + >> + now = next; >> + iter = niter; >> + } >> + >> + return max; >> +#else >> + return 0; >> +#endif >> +} >> +EXPORT_SYMBOL_GPL(netdev_get_nest_level_rcu); >> + >> int netdev_walk_all_lower_dev_rcu(struct net_device *dev, >> int (*fn)(struct net_device *dev, >> struct netdev_nested_priv *priv), >> > I think you're right. Your logic is more simple and stable. If I send a v3 patch, I will use your logic after some tests. Thanks a lot! Taehee
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 83ef62db6285..a9feb039ffa6 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -3754,47 +3754,6 @@ static void bond_fold_stats(struct rtnl_link_stats64 *_res, } } -#ifdef CONFIG_LOCKDEP -static int bond_get_lowest_level_rcu(struct net_device *dev) -{ - struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1]; - struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1]; - int cur = 0, max = 0; - - now = dev; - iter = &dev->adj_list.lower; - - while (1) { - next = NULL; - while (1) { - ldev = netdev_next_lower_dev_rcu(now, &iter); - if (!ldev) - break; - - next = ldev; - niter = &ldev->adj_list.lower; - dev_stack[cur] = now; - iter_stack[cur++] = iter; - if (max <= cur) - max = cur; - break; - } - - if (!next) { - if (!cur) - return max; - next = dev_stack[--cur]; - niter = iter_stack[cur]; - } - - now = next; - iter = niter; - } - - return max; -} -#endif - static void bond_get_stats(struct net_device *bond_dev, struct rtnl_link_stats64 *stats) { @@ -3806,9 +3765,7 @@ static void bond_get_stats(struct net_device *bond_dev, rcu_read_lock(); -#ifdef CONFIG_LOCKDEP - nest_level = bond_get_lowest_level_rcu(bond_dev); -#endif + nest_level = netdev_get_nest_level_rcu(bond_dev); spin_lock_nested(&bond->stats_lock, nest_level); memcpy(stats, &bond->bond_stats, sizeof(*stats)); diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 87a5d186faff..507c06bf5dba 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -4699,6 +4699,7 @@ int netdev_walk_all_lower_dev(struct net_device *dev, int (*fn)(struct net_device *lower_dev, struct netdev_nested_priv *priv), struct netdev_nested_priv *priv); +int netdev_get_nest_level_rcu(struct net_device *dev); int netdev_walk_all_lower_dev_rcu(struct net_device *dev, int (*fn)(struct net_device *lower_dev, struct netdev_nested_priv *priv), diff --git a/net/core/dev.c b/net/core/dev.c index 15fe36332fb8..efc2bf88eafd 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -7709,6 +7709,50 @@ static int __netdev_update_lower_level(struct net_device *dev, return 0; } +int netdev_get_nest_level_rcu(struct net_device *dev) +{ +#ifdef CONFIG_LOCKDEP + struct net_device *ldev, *next, *now, *dev_stack[MAX_NEST_DEV + 1]; + struct list_head *niter, *iter, *iter_stack[MAX_NEST_DEV + 1]; + int cur = 0, max = 0; + + now = dev; + iter = &dev->adj_list.lower; + + while (1) { + next = NULL; + while (1) { + ldev = netdev_next_lower_dev_rcu(now, &iter); + if (!ldev) + break; + + next = ldev; + niter = &ldev->adj_list.lower; + dev_stack[cur] = now; + iter_stack[cur++] = iter; + if (max <= cur) + max = cur; + break; + } + + if (!next) { + if (!cur) + return max; + next = dev_stack[--cur]; + niter = iter_stack[cur]; + } + + now = next; + iter = niter; + } + + return max; +#else + return 0; +#endif +} +EXPORT_SYMBOL_GPL(netdev_get_nest_level_rcu); + int netdev_walk_all_lower_dev_rcu(struct net_device *dev, int (*fn)(struct net_device *dev, struct netdev_nested_priv *priv),
The purpose of bond_get_lowest_level_rcu() is to get nested_level under RCU. Because dev->nested_level is protected by RTNL, so it shouldn't be used without RTNL. But bonding module needs this value under RCU without RTNL. So, this function was added. But, there is another module, which needs this function. So, make this function generic. the new name is netdev_get_nest_level_rcu(). Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- v2: - No change drivers/net/bonding/bond_main.c | 45 +-------------------------------- include/linux/netdevice.h | 1 + net/core/dev.c | 44 ++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 44 deletions(-)