[PATCH net-next 0/5] bonding: do some cleanups in bond driver

[PATCH net-next 0/5] bonding: do some cleanups in bond driver

[Zhengchao Shao]

Do some cleanups in bond driver.

Zhengchao Shao (5):
  bonding: add modifier to initialization function and exit function
  bonding: remove warning printing in bond_create_debugfs
  bonding: remove unnecessary NULL check in debugfs function
  bonding: use bond_set_slave_arr to simplify code
  bonding: remove unnecessary NULL check in bond_destructor

 drivers/net/bonding/bond_debugfs.c | 16 ++------------
 drivers/net/bonding/bond_main.c    | 34 +++++-------------------------
 drivers/net/bonding/bond_sysfs.c   |  4 ++--
 3 files changed, 9 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCH net-next 1/5] bonding: add modifier to initialization function and exit function

[Zhengchao Shao]

Some functions are only used in initialization and exit functions, so add
the __init/__net_init and __net_exit modifiers to these functions.

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 drivers/net/bonding/bond_debugfs.c | 4 ++--
 drivers/net/bonding/bond_main.c    | 2 +-
 drivers/net/bonding/bond_sysfs.c   | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_debugfs.c b/drivers/net/bonding/bond_debugfs.c
index 594094526648..94c2f35e3bfc 100644
--- a/drivers/net/bonding/bond_debugfs.c
+++ b/drivers/net/bonding/bond_debugfs.c
@@ -84,7 +84,7 @@ void bond_debug_reregister(struct bonding *bond)
 	}
 }
 
-void bond_create_debugfs(void)
+void __init bond_create_debugfs(void)
 {
 	bonding_debug_root = debugfs_create_dir("bonding", NULL);
 
@@ -113,7 +113,7 @@ void bond_debug_reregister(struct bonding *bond)
 {
 }
 
-void bond_create_debugfs(void)
+void __init bond_create_debugfs(void)
 {
 }
 
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d26c69d84c1e..6636638f5d97 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5981,7 +5981,7 @@ static void bond_uninit(struct net_device *bond_dev)
 
 /*------------------------- Module initialization ---------------------------*/
 
-static int bond_check_params(struct bond_params *params)
+static int __init bond_check_params(struct bond_params *params)
 {
 	int arp_validate_value, fail_over_mac_value, primary_reselect_value, i;
 	struct bond_opt_value newval;
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 0bb59da24922..2805135a7205 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -803,7 +803,7 @@ static const struct attribute_group bonding_group = {
 /* Initialize sysfs.  This sets up the bonding_masters file in
  * /sys/class/net.
  */
-int bond_create_sysfs(struct bond_net *bn)
+int __net_init bond_create_sysfs(struct bond_net *bn)
 {
 	int ret;
 
@@ -836,7 +836,7 @@ int bond_create_sysfs(struct bond_net *bn)
 }
 
 /* Remove /sys/class/net/bonding_masters. */
-void bond_destroy_sysfs(struct bond_net *bn)
+void __net_exit bond_destroy_sysfs(struct bond_net *bn)
 {
 	netdev_class_remove_file_ns(&bn->class_attr_bonding_masters, bn->net);
 }
-- 
2.34.1

Re: [PATCH net-next 1/5] bonding: add modifier to initialization function and exit function

[Leon Romanovsky]

On Wed, Aug 09, 2023 at 08:41:03PM +0800, Zhengchao Shao wrote:
> Some functions are only used in initialization and exit functions, so add
> the __init/__net_init and __net_exit modifiers to these functions.
> 
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
>  drivers/net/bonding/bond_debugfs.c | 4 ++--
>  drivers/net/bonding/bond_main.c    | 2 +-
>  drivers/net/bonding/bond_sysfs.c   | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

[PATCH net-next 2/5] bonding: remove warning printing in bond_create_debugfs

[Zhengchao Shao]

Because debugfs_create_dir returns ERR_PTR, so warning printing will never
be invoked in bond_create_debugfs, remove it. If failed to create
directory, failure information will be printed in debugfs_create_dir.

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 drivers/net/bonding/bond_debugfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/bonding/bond_debugfs.c b/drivers/net/bonding/bond_debugfs.c
index 94c2f35e3bfc..e4e7f4ee48e0 100644
--- a/drivers/net/bonding/bond_debugfs.c
+++ b/drivers/net/bonding/bond_debugfs.c
@@ -87,9 +87,6 @@ void bond_debug_reregister(struct bonding *bond)
 void __init bond_create_debugfs(void)
 {
 	bonding_debug_root = debugfs_create_dir("bonding", NULL);
-
-	if (!bonding_debug_root)
-		pr_warn("Warning: Cannot create bonding directory in debugfs\n");
 }
 
 void bond_destroy_debugfs(void)
-- 
2.34.1

Re: [PATCH net-next 2/5] bonding: remove warning printing in bond_create_debugfs

[Hangbin Liu]

On Wed, Aug 09, 2023 at 08:41:04PM +0800, Zhengchao Shao wrote:
> Because debugfs_create_dir returns ERR_PTR, so warning printing will never
> be invoked in bond_create_debugfs, remove it. If failed to create
> directory, failure information will be printed in debugfs_create_dir.
> 
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
>  drivers/net/bonding/bond_debugfs.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_debugfs.c b/drivers/net/bonding/bond_debugfs.c
> index 94c2f35e3bfc..e4e7f4ee48e0 100644
> --- a/drivers/net/bonding/bond_debugfs.c
> +++ b/drivers/net/bonding/bond_debugfs.c
> @@ -87,9 +87,6 @@ void bond_debug_reregister(struct bonding *bond)
>  void __init bond_create_debugfs(void)
>  {
>  	bonding_debug_root = debugfs_create_dir("bonding", NULL);
> -
> -	if (!bonding_debug_root)

debugfs_create_dir() does not print information for all failures. We can use
IS_ERR(bonding_debug_root) to check the value here.

Thanks
Hangbin
> -		pr_warn("Warning: Cannot create bonding directory in debugfs\n");
>  }
>  
>  void bond_destroy_debugfs(void)
> -- 
> 2.34.1
> 

Re: [PATCH net-next 2/5] bonding: remove warning printing in bond_create_debugfs

[shaozhengchao]

On 2023/8/10 10:53, Hangbin Liu wrote:
> On Wed, Aug 09, 2023 at 08:41:04PM +0800, Zhengchao Shao wrote:
>> Because debugfs_create_dir returns ERR_PTR, so warning printing will never
>> be invoked in bond_create_debugfs, remove it. If failed to create
>> directory, failure information will be printed in debugfs_create_dir.
>>
>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
>> ---
>>   drivers/net/bonding/bond_debugfs.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_debugfs.c b/drivers/net/bonding/bond_debugfs.c
>> index 94c2f35e3bfc..e4e7f4ee48e0 100644
>> --- a/drivers/net/bonding/bond_debugfs.c
>> +++ b/drivers/net/bonding/bond_debugfs.c
>> @@ -87,9 +87,6 @@ void bond_debug_reregister(struct bonding *bond)
>>   void __init bond_create_debugfs(void)
>>   {
>>   	bonding_debug_root = debugfs_create_dir("bonding", NULL);
>> -
>> -	if (!bonding_debug_root)
> 
Hi Hangbin:
> debugfs_create_dir() does not print information for all failures. We can use
> IS_ERR(bonding_debug_root) to check the value here.
> 
	Thank you for your review. I think you are right here, and I
will modify it.

Zhengchao Shao
> Thanks
> Hangbin
>> -		pr_warn("Warning: Cannot create bonding directory in debugfs\n");
>>   }
>>   
>>   void bond_destroy_debugfs(void)
>> -- 
>> 2.34.1
>>

[PATCH net-next 3/5] bonding: remove unnecessary NULL check in debugfs function

[Zhengchao Shao]

Because debugfs_create_dir returns ERR_PTR, so bonding_debug_root will
never be NULL. Remove unnecessary NULL check for bonding_debug_root in
debugfs function.

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 drivers/net/bonding/bond_debugfs.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/net/bonding/bond_debugfs.c b/drivers/net/bonding/bond_debugfs.c
index e4e7f4ee48e0..4c83f89c0a47 100644
--- a/drivers/net/bonding/bond_debugfs.c
+++ b/drivers/net/bonding/bond_debugfs.c
@@ -49,9 +49,6 @@ DEFINE_SHOW_ATTRIBUTE(bond_debug_rlb_hash);
 
 void bond_debug_register(struct bonding *bond)
 {
-	if (!bonding_debug_root)
-		return;
-
 	bond->debug_dir =
 		debugfs_create_dir(bond->dev->name, bonding_debug_root);
 
@@ -61,9 +58,6 @@ void bond_debug_register(struct bonding *bond)
 
 void bond_debug_unregister(struct bonding *bond)
 {
-	if (!bonding_debug_root)
-		return;
-
 	debugfs_remove_recursive(bond->debug_dir);
 }
 
@@ -71,9 +65,6 @@ void bond_debug_reregister(struct bonding *bond)
 {
 	struct dentry *d;
 
-	if (!bonding_debug_root)
-		return;
-
 	d = debugfs_rename(bonding_debug_root, bond->debug_dir,
 			   bonding_debug_root, bond->dev->name);
 	if (!IS_ERR(d)) {
-- 
2.34.1

Re: [PATCH net-next 3/5] bonding: remove unnecessary NULL check in debugfs function

[Jay Vosburgh]

On 8/9/23, Zhengchao Shao <shaozhengchao@huawei.com> wrote:
> Because debugfs_create_dir returns ERR_PTR, so bonding_debug_root will
> never be NULL. Remove unnecessary NULL check for bonding_debug_root in
> debugfs function.

So after this change it will call debugfs_create_dir(), et al, with
the ERR_PTR value?  Granted, the current behavior is probably not
right, but I don't see how this makes things better.

-J

>
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
>  drivers/net/bonding/bond_debugfs.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_debugfs.c
> b/drivers/net/bonding/bond_debugfs.c
> index e4e7f4ee48e0..4c83f89c0a47 100644
> --- a/drivers/net/bonding/bond_debugfs.c
> +++ b/drivers/net/bonding/bond_debugfs.c
> @@ -49,9 +49,6 @@ DEFINE_SHOW_ATTRIBUTE(bond_debug_rlb_hash);
>
>  void bond_debug_register(struct bonding *bond)
>  {
> -	if (!bonding_debug_root)
> -		return;
> -
>  	bond->debug_dir =
>  		debugfs_create_dir(bond->dev->name, bonding_debug_root);
>
> @@ -61,9 +58,6 @@ void bond_debug_register(struct bonding *bond)
>
>  void bond_debug_unregister(struct bonding *bond)
>  {
> -	if (!bonding_debug_root)
> -		return;
> -
>  	debugfs_remove_recursive(bond->debug_dir);
>  }
>
> @@ -71,9 +65,6 @@ void bond_debug_reregister(struct bonding *bond)
>  {
>  	struct dentry *d;
>
> -	if (!bonding_debug_root)
> -		return;
> -
>  	d = debugfs_rename(bonding_debug_root, bond->debug_dir,
>  			   bonding_debug_root, bond->dev->name);
>  	if (!IS_ERR(d)) {
> --
> 2.34.1
>
>

Re: [PATCH net-next 3/5] bonding: remove unnecessary NULL check in debugfs function

[Hangbin Liu]

On Wed, Aug 09, 2023 at 09:13:31AM -0700, Jay Vosburgh wrote:
> On 8/9/23, Zhengchao Shao <shaozhengchao@huawei.com> wrote:
> > Because debugfs_create_dir returns ERR_PTR, so bonding_debug_root will
> > never be NULL. Remove unnecessary NULL check for bonding_debug_root in
> > debugfs function.
> 
> So after this change it will call debugfs_create_dir(), et al, with
> the ERR_PTR value?  Granted, the current behavior is probably not
> right, but I don't see how this makes things better.

I guess Zhengchao means to remove Redundant checks? The later
debugfs_create_dir/debugfs_remove_recursive/debugfs_remove_recursive functions
will check the dentry with IS_ERR(). But I think the commit description need
an update.

Hangbin

> 
> -J
> 
> >
> > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> > ---
> >  drivers/net/bonding/bond_debugfs.c | 9 ---------
> >  1 file changed, 9 deletions(-)
> >
> > diff --git a/drivers/net/bonding/bond_debugfs.c
> > b/drivers/net/bonding/bond_debugfs.c
> > index e4e7f4ee48e0..4c83f89c0a47 100644
> > --- a/drivers/net/bonding/bond_debugfs.c
> > +++ b/drivers/net/bonding/bond_debugfs.c
> > @@ -49,9 +49,6 @@ DEFINE_SHOW_ATTRIBUTE(bond_debug_rlb_hash);
> >
> >  void bond_debug_register(struct bonding *bond)
> >  {
> > -	if (!bonding_debug_root)
> > -		return;
> > -
> >  	bond->debug_dir =
> >  		debugfs_create_dir(bond->dev->name, bonding_debug_root);
> >
> > @@ -61,9 +58,6 @@ void bond_debug_register(struct bonding *bond)
> >
> >  void bond_debug_unregister(struct bonding *bond)
> >  {
> > -	if (!bonding_debug_root)
> > -		return;
> > -
> >  	debugfs_remove_recursive(bond->debug_dir);
> >  }
> >
> > @@ -71,9 +65,6 @@ void bond_debug_reregister(struct bonding *bond)
> >  {
> >  	struct dentry *d;
> >
> > -	if (!bonding_debug_root)
> > -		return;
> > -
> >  	d = debugfs_rename(bonding_debug_root, bond->debug_dir,
> >  			   bonding_debug_root, bond->dev->name);
> >  	if (!IS_ERR(d)) {
> > --
> > 2.34.1
> >
> >

Re: [PATCH net-next 3/5] bonding: remove unnecessary NULL check in debugfs function

[shaozhengchao]

On 2023/8/10 11:08, Hangbin Liu wrote:
> On Wed, Aug 09, 2023 at 09:13:31AM -0700, Jay Vosburgh wrote:
>> On 8/9/23, Zhengchao Shao <shaozhengchao@huawei.com> wrote:
>>> Because debugfs_create_dir returns ERR_PTR, so bonding_debug_root will
>>> never be NULL. Remove unnecessary NULL check for bonding_debug_root in
>>> debugfs function.
>>
>> So after this change it will call debugfs_create_dir(), et al, with
>> the ERR_PTR value?  Granted, the current behavior is probably not
>> right, but I don't see how this makes things better.
> 
> I guess Zhengchao means to remove Redundant checks? The later
> debugfs_create_dir/debugfs_remove_recursive/debugfs_remove_recursive functions
> will check the dentry with IS_ERR(). But I think the commit description need
> an update.
> 
> Hangbin
> 
Yes, I will update this commit description.

Zhengchao Shao
>>
>> -J
>>
>>>
>>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
>>> ---
>>>   drivers/net/bonding/bond_debugfs.c | 9 ---------
>>>   1 file changed, 9 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_debugfs.c
>>> b/drivers/net/bonding/bond_debugfs.c
>>> index e4e7f4ee48e0..4c83f89c0a47 100644
>>> --- a/drivers/net/bonding/bond_debugfs.c
>>> +++ b/drivers/net/bonding/bond_debugfs.c
>>> @@ -49,9 +49,6 @@ DEFINE_SHOW_ATTRIBUTE(bond_debug_rlb_hash);
>>>
>>>   void bond_debug_register(struct bonding *bond)
>>>   {
>>> -	if (!bonding_debug_root)
>>> -		return;
>>> -
>>>   	bond->debug_dir =
>>>   		debugfs_create_dir(bond->dev->name, bonding_debug_root);
>>>
>>> @@ -61,9 +58,6 @@ void bond_debug_register(struct bonding *bond)
>>>
>>>   void bond_debug_unregister(struct bonding *bond)
>>>   {
>>> -	if (!bonding_debug_root)
>>> -		return;
>>> -
>>>   	debugfs_remove_recursive(bond->debug_dir);
>>>   }
>>>
>>> @@ -71,9 +65,6 @@ void bond_debug_reregister(struct bonding *bond)
>>>   {
>>>   	struct dentry *d;
>>>
>>> -	if (!bonding_debug_root)
>>> -		return;
>>> -
>>>   	d = debugfs_rename(bonding_debug_root, bond->debug_dir,
>>>   			   bonding_debug_root, bond->dev->name);
>>>   	if (!IS_ERR(d)) {
>>> --
>>> 2.34.1
>>>
>>>

[PATCH net-next 4/5] bonding: use bond_set_slave_arr to simplify code

[Zhengchao Shao]

In bond_reset_slave_arr(), values are assigned and memory is released only
when the variables "usable" and "all" are not NULL. But even if the
"usable" and "all" variables are NULL, they can still work, because value
will be checked in kfree_rcu. Therefore, use bond_set_slave_arr() and set
the input parameters "usable_slaves" and "all_slaves" to NULL to simplify
the code in bond_reset_slave_arr(). And the same to bond_uninit().

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 drivers/net/bonding/bond_main.c | 29 +++--------------------------
 1 file changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6636638f5d97..dcc67bd4d5cf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5044,21 +5044,9 @@ static void bond_set_slave_arr(struct bonding *bond,
 	kfree_rcu(all, rcu);
 }
 
-static void bond_reset_slave_arr(struct bonding *bond)
+static inline void bond_reset_slave_arr(struct bonding *bond)
 {
-	struct bond_up_slave *usable, *all;
-
-	usable = rtnl_dereference(bond->usable_slaves);
-	if (usable) {
-		RCU_INIT_POINTER(bond->usable_slaves, NULL);
-		kfree_rcu(usable, rcu);
-	}
-
-	all = rtnl_dereference(bond->all_slaves);
-	if (all) {
-		RCU_INIT_POINTER(bond->all_slaves, NULL);
-		kfree_rcu(all, rcu);
-	}
+	bond_set_slave_arr(bond, NULL, NULL);
 }
 
 /* Build the usable slaves array in control path for modes that use xmit-hash
@@ -5951,7 +5939,6 @@ void bond_setup(struct net_device *bond_dev)
 static void bond_uninit(struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
-	struct bond_up_slave *usable, *all;
 	struct list_head *iter;
 	struct slave *slave;
 
@@ -5962,17 +5949,7 @@ static void bond_uninit(struct net_device *bond_dev)
 		__bond_release_one(bond_dev, slave->dev, true, true);
 	netdev_info(bond_dev, "Released all slaves\n");
 
-	usable = rtnl_dereference(bond->usable_slaves);
-	if (usable) {
-		RCU_INIT_POINTER(bond->usable_slaves, NULL);
-		kfree_rcu(usable, rcu);
-	}
-
-	all = rtnl_dereference(bond->all_slaves);
-	if (all) {
-		RCU_INIT_POINTER(bond->all_slaves, NULL);
-		kfree_rcu(all, rcu);
-	}
+	bond_set_slave_arr(bond, NULL, NULL);
 
 	list_del(&bond->bond_list);
 
-- 
2.34.1

Re: [PATCH net-next 4/5] bonding: use bond_set_slave_arr to simplify code

[Vadim Fedorenko]

On 09/08/2023 13:41, Zhengchao Shao wrote:
> In bond_reset_slave_arr(), values are assigned and memory is released only
> when the variables "usable" and "all" are not NULL. But even if the
> "usable" and "all" variables are NULL, they can still work, because value
> will be checked in kfree_rcu. Therefore, use bond_set_slave_arr() and set
> the input parameters "usable_slaves" and "all_slaves" to NULL to simplify
> the code in bond_reset_slave_arr(). And the same to bond_uninit().
> 
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
>   drivers/net/bonding/bond_main.c | 29 +++--------------------------
>   1 file changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 6636638f5d97..dcc67bd4d5cf 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5044,21 +5044,9 @@ static void bond_set_slave_arr(struct bonding *bond,
>   	kfree_rcu(all, rcu);
>   }
>   
> -static void bond_reset_slave_arr(struct bonding *bond)
> +static inline void bond_reset_slave_arr(struct bonding *bond)

No explicit inline in c files. Remove it and let the compiler decide.

>   {
> -	struct bond_up_slave *usable, *all;
> -
> -	usable = rtnl_dereference(bond->usable_slaves);
> -	if (usable) {
> -		RCU_INIT_POINTER(bond->usable_slaves, NULL);
> -		kfree_rcu(usable, rcu);
> -	}
> -
> -	all = rtnl_dereference(bond->all_slaves);
> -	if (all) {
> -		RCU_INIT_POINTER(bond->all_slaves, NULL);
> -		kfree_rcu(all, rcu);
> -	}
> +	bond_set_slave_arr(bond, NULL, NULL);
>   }
>   
>   /* Build the usable slaves array in control path for modes that use xmit-hash
> @@ -5951,7 +5939,6 @@ void bond_setup(struct net_device *bond_dev)
>   static void bond_uninit(struct net_device *bond_dev)
>   {
>   	struct bonding *bond = netdev_priv(bond_dev);
> -	struct bond_up_slave *usable, *all;
>   	struct list_head *iter;
>   	struct slave *slave;
>   
> @@ -5962,17 +5949,7 @@ static void bond_uninit(struct net_device *bond_dev)
>   		__bond_release_one(bond_dev, slave->dev, true, true);
>   	netdev_info(bond_dev, "Released all slaves\n");
>   
> -	usable = rtnl_dereference(bond->usable_slaves);
> -	if (usable) {
> -		RCU_INIT_POINTER(bond->usable_slaves, NULL);
> -		kfree_rcu(usable, rcu);
> -	}
> -
> -	all = rtnl_dereference(bond->all_slaves);
> -	if (all) {
> -		RCU_INIT_POINTER(bond->all_slaves, NULL);
> -		kfree_rcu(all, rcu);
> -	}
> +	bond_set_slave_arr(bond, NULL, NULL);
>   
>   	list_del(&bond->bond_list);
>   
-- 
pw-bot: cr

Re: [PATCH net-next 4/5] bonding: use bond_set_slave_arr to simplify code

[shaozhengchao]

On 2023/8/10 8:17, Vadim Fedorenko wrote:
> On 09/08/2023 13:41, Zhengchao Shao wrote:
>> In bond_reset_slave_arr(), values are assigned and memory is released 
>> only
>> when the variables "usable" and "all" are not NULL. But even if the
>> "usable" and "all" variables are NULL, they can still work, because value
>> will be checked in kfree_rcu. Therefore, use bond_set_slave_arr() and set
>> the input parameters "usable_slaves" and "all_slaves" to NULL to simplify
>> the code in bond_reset_slave_arr(). And the same to bond_uninit().
>>
>> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
>> ---
>>   drivers/net/bonding/bond_main.c | 29 +++--------------------------
>>   1 file changed, 3 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c 
>> b/drivers/net/bonding/bond_main.c
>> index 6636638f5d97..dcc67bd4d5cf 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -5044,21 +5044,9 @@ static void bond_set_slave_arr(struct bonding 
>> *bond,
>>       kfree_rcu(all, rcu);
>>   }
>> -static void bond_reset_slave_arr(struct bonding *bond)
>> +static inline void bond_reset_slave_arr(struct bonding *bond)
> 
> No explicit inline in c files. Remove it and let the compiler decide.
> 
Hi Vadim:
	Thank you for your review. I will remove it in v2.

Zhengchao Shao
>>   {
>> -    struct bond_up_slave *usable, *all;
>> -
>> -    usable = rtnl_dereference(bond->usable_slaves);
>> -    if (usable) {
>> -        RCU_INIT_POINTER(bond->usable_slaves, NULL);
>> -        kfree_rcu(usable, rcu);
>> -    }
>> -
>> -    all = rtnl_dereference(bond->all_slaves);
>> -    if (all) {
>> -        RCU_INIT_POINTER(bond->all_slaves, NULL);
>> -        kfree_rcu(all, rcu);
>> -    }
>> +    bond_set_slave_arr(bond, NULL, NULL);
>>   }
>>   /* Build the usable slaves array in control path for modes that use 
>> xmit-hash
>> @@ -5951,7 +5939,6 @@ void bond_setup(struct net_device *bond_dev)
>>   static void bond_uninit(struct net_device *bond_dev)
>>   {
>>       struct bonding *bond = netdev_priv(bond_dev);
>> -    struct bond_up_slave *usable, *all;
>>       struct list_head *iter;
>>       struct slave *slave;
>> @@ -5962,17 +5949,7 @@ static void bond_uninit(struct net_device 
>> *bond_dev)
>>           __bond_release_one(bond_dev, slave->dev, true, true);
>>       netdev_info(bond_dev, "Released all slaves\n");
>> -    usable = rtnl_dereference(bond->usable_slaves);
>> -    if (usable) {
>> -        RCU_INIT_POINTER(bond->usable_slaves, NULL);
>> -        kfree_rcu(usable, rcu);
>> -    }
>> -
>> -    all = rtnl_dereference(bond->all_slaves);
>> -    if (all) {
>> -        RCU_INIT_POINTER(bond->all_slaves, NULL);
>> -        kfree_rcu(all, rcu);
>> -    }
>> +    bond_set_slave_arr(bond, NULL, NULL);
>>       list_del(&bond->bond_list);

[PATCH net-next 5/5] bonding: remove unnecessary NULL check in bond_destructor

[Zhengchao Shao]

The free_percpu function also could check whether "rr_tx_counter"
parameter is NULL. Therefore, remove NULL check in bond_destructor.

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 drivers/net/bonding/bond_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index dcc67bd4d5cf..aa77580bd9ad 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5863,8 +5863,7 @@ static void bond_destructor(struct net_device *bond_dev)
 	if (bond->wq)
 		destroy_workqueue(bond->wq);
 
-	if (bond->rr_tx_counter)
-		free_percpu(bond->rr_tx_counter);
+	free_percpu(bond->rr_tx_counter);
 }
 
 void bond_setup(struct net_device *bond_dev)
-- 
2.34.1

Re: [PATCH net-next 5/5] bonding: remove unnecessary NULL check in bond_destructor

[Leon Romanovsky]

On Wed, Aug 09, 2023 at 08:41:07PM +0800, Zhengchao Shao wrote:
> The free_percpu function also could check whether "rr_tx_counter"
> parameter is NULL. Therefore, remove NULL check in bond_destructor.
> 
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
>  drivers/net/bonding/bond_main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>