| Message ID | 20230809124107.360574-5-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bonding: do some cleanups in bond driver | expand |
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); >
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);
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);
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(-)