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 <[email protected]>
>> ---
>>
>> 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
