| Message ID | 20130720104746.GC9149@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 07/20/2013 12:47 PM, Veaceslav Falico wrote: > On Sat, Jul 20, 2013 at 03:23:57PM +0800, dingtianhong wrote: >> The bonding_store_mode has rtnl protection, so no need to get read lock >> for bond->slave_cnt, but the bonding_store_fail_over_mac need to protect >> the bond->slave_cnt, so add read_lock(). >> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >> <snip> > > Maybe it's Saturday, but I really don't see *any* point in this locking. > > I think you've meant that we need the rtnl protection while reading > slave_cnt AND updating the .fail_over_mac, so that in between we won't add > new slaves with outdated params. > > Something like this (untested): > Indeed, Veaceslav's way is the correct one (I've looked at this race before), but IMO it's not worth it to protect fail_over_mac as the worst that could happen is inconsistency with the MAC addresses which isn't fatal. Anyway, I still haven't had my coffee and might be missing something :-) Cheers, Nik -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 20, 2013 at 02:42:37PM +0200, Nikolay Aleksandrov wrote: >On 07/20/2013 12:47 PM, Veaceslav Falico wrote: >> On Sat, Jul 20, 2013 at 03:23:57PM +0800, dingtianhong wrote: >>> The bonding_store_mode has rtnl protection, so no need to get read lock >>> for bond->slave_cnt, but the bonding_store_fail_over_mac need to protect >>> the bond->slave_cnt, so add read_lock(). >>> >>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >>> ><snip> >> >> Maybe it's Saturday, but I really don't see *any* point in this locking. >> >> I think you've meant that we need the rtnl protection while reading >> slave_cnt AND updating the .fail_over_mac, so that in between we won't add >> new slaves with outdated params. >> >> Something like this (untested): >> >Indeed, Veaceslav's way is the correct one (I've looked at this race >before), but IMO it's not worth it to protect fail_over_mac as the worst >that could happen is inconsistency with the MAC addresses which isn't >fatal. Anyway, I still haven't had my coffee and might be missing something :-) Yep, agree that it's kind of minor and hard to hit in real life. OTOH, getting the rtnl here costs us virtually nothing and might save someone from a headache :). And it also follows the logic "don't change anything slave-related without rtnl". So I'd rather have it, as a minor improvement :). > >Cheers, > Nik > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/7/20 18:47, Veaceslav Falico wrote: > On Sat, Jul 20, 2013 at 03:23:57PM +0800, dingtianhong wrote: >> The bonding_store_mode has rtnl protection, so no need to get read lock >> for bond->slave_cnt, but the bonding_store_fail_over_mac need to protect >> the bond->slave_cnt, so add read_lock(). >> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >> >> --- >> drivers/net/bonding/bond_sysfs.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c >> index dc36a3d..d01a189 100644 >> --- a/drivers/net/bonding/bond_sysfs.c >> +++ b/drivers/net/bonding/bond_sysfs.c >> @@ -504,11 +504,14 @@ static ssize_t bonding_store_fail_over_mac(struct device *d, >> int new_value; >> struct bonding *bond = to_bond(d); >> >> + read_lock(&bond->lock); >> if (bond->slave_cnt != 0) { >> pr_err("%s: Can't alter fail_over_mac with slaves in bond.\n", >> bond->dev->name); >> + read_unlock(&bond->lock); >> return -EPERM; >> } >> + read_unlock(&bond->lock); > > Maybe it's Saturday, but I really don't see *any* point in this locking. > > I think you've meant that we need the rtnl protection while reading > slave_cnt AND updating the .fail_over_mac, so that in between we won't add > new slaves with outdated params. yes, as you said, the rtnl lock is enough here, but I think rtnl lock is bigger than a readlock, so think about the performance, i choose the readlock to protect slave_cnt. > > Something like this (untested): > > diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c > index dc36a3d..8a5a6a3 100644 > --- a/drivers/net/bonding/bond_sysfs.c > +++ b/drivers/net/bonding/bond_sysfs.c > @@ -501,20 +501,25 @@ static ssize_t bonding_store_fail_over_mac(struct device *d, > struct device_attribute *attr, > const char *buf, size_t count) > { > - int new_value; > + int new_value, ret = count; > struct bonding *bond = to_bond(d); > > + if (!rtnl_trylock()) > + return restart_syscall(); > + > if (bond->slave_cnt != 0) { > pr_err("%s: Can't alter fail_over_mac with slaves in bond.\n", > bond->dev->name); > - return -EPERM; > + ret = -EPERM; > + goto out; > } > > new_value = bond_parse_parm(buf, fail_over_mac_tbl); > if (new_value < 0) { > pr_err("%s: Ignoring invalid fail_over_mac value %s.\n", > bond->dev->name, buf); > - return -EINVAL; > + ret = -EINVAL; > + goto out; > } > > bond->params.fail_over_mac = new_value; > @@ -522,7 +527,9 @@ static ssize_t bonding_store_fail_over_mac(struct device *d, > bond->dev->name, fail_over_mac_tbl[new_value].modename, > new_value); > > - return count; > +out: > + rtnl_unlock(); > + return ret; > } > > static DEVICE_ATTR(fail_over_mac, S_IRUGO | S_IWUSR, >> >> new_value = bond_parse_parm(buf, fail_over_mac_tbl); >> if (new_value < 0) { > > . > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/7/20 23:00, Veaceslav Falico wrote: > On Sat, Jul 20, 2013 at 02:42:37PM +0200, Nikolay Aleksandrov wrote: >> On 07/20/2013 12:47 PM, Veaceslav Falico wrote: >>> On Sat, Jul 20, 2013 at 03:23:57PM +0800, dingtianhong wrote: >>>> The bonding_store_mode has rtnl protection, so no need to get read lock >>>> for bond->slave_cnt, but the bonding_store_fail_over_mac need to protect >>>> the bond->slave_cnt, so add read_lock(). >>>> >>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> >>>> >> <snip> >>> >>> Maybe it's Saturday, but I really don't see *any* point in this locking. >>> >>> I think you've meant that we need the rtnl protection while reading >>> slave_cnt AND updating the .fail_over_mac, so that in between we won't add >>> new slaves with outdated params. >>> >>> Something like this (untested): >>> >> Indeed, Veaceslav's way is the correct one (I've looked at this race >> before), but IMO it's not worth it to protect fail_over_mac as the worst >> that could happen is inconsistency with the MAC addresses which isn't >> fatal. Anyway, I still haven't had my coffee and might be missing something :-) > > Yep, agree that it's kind of minor and hard to hit in real life. > > OTOH, getting the rtnl here costs us virtually nothing and might save > someone from a headache :). And it also follows the logic "don't change > anything slave-related without rtnl". > > So I'd rather have it, as a minor improvement :). > >> >> Cheers, >> Nik yes , i think it is hard to hit the problem in real lift, just looks better. :) >> > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index dc36a3d..8a5a6a3 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -501,20 +501,25 @@ static ssize_t bonding_store_fail_over_mac(struct device *d, struct device_attribute *attr, const char *buf, size_t count) { - int new_value; + int new_value, ret = count; struct bonding *bond = to_bond(d); + if (!rtnl_trylock()) + return restart_syscall(); + if (bond->slave_cnt != 0) { pr_err("%s: Can't alter fail_over_mac with slaves in bond.\n", bond->dev->name); - return -EPERM; + ret = -EPERM; + goto out; } new_value = bond_parse_parm(buf, fail_over_mac_tbl); if (new_value < 0) { pr_err("%s: Ignoring invalid fail_over_mac value %s.\n", bond->dev->name, buf); - return -EINVAL; + ret = -EINVAL; + goto out; } bond->params.fail_over_mac = new_value; @@ -522,7 +527,9 @@ static ssize_t bonding_store_fail_over_mac(struct device *d, bond->dev->name, fail_over_mac_tbl[new_value].modename, new_value); - return count; +out: + rtnl_unlock(); + return ret; } static DEVICE_ATTR(fail_over_mac, S_IRUGO | S_IWUSR,