* [PATCH net-next] bonding: use rtnl_deref in bond_change_rx_flags() @ 2014-07-16 7:24 Veaceslav Falico 2014-07-16 9:05 ` Eric Dumazet 0 siblings, 1 reply; 3+ messages in thread From: Veaceslav Falico @ 2014-07-16 7:24 UTC (permalink / raw) To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek As it's always called with RTNL held, via dev_set_allmulti/promiscuity. CC: Jay Vosburgh <j.vosburgh@gmail.com> CC: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Veaceslav Falico <vfalico@gmail.com> --- drivers/net/bonding/bond_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index d643807..2998710 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -498,7 +498,7 @@ static int bond_set_promiscuity(struct bonding *bond, int inc) int err = 0; if (bond_uses_primary(bond)) { - struct slave *curr_active = bond_deref_active_protected(bond); + struct slave *curr_active = rtnl_dereference(bond->curr_active_slave); /* write lock already acquired */ if (curr_active) @@ -524,7 +524,7 @@ static int bond_set_allmulti(struct bonding *bond, int inc) int err = 0; if (bond_uses_primary(bond)) { - struct slave *curr_active = bond_deref_active_protected(bond); + struct slave *curr_active = rtnl_dereference(bond->curr_active_slave); /* write lock already acquired */ if (curr_active) -- 1.8.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] bonding: use rtnl_deref in bond_change_rx_flags() 2014-07-16 7:24 [PATCH net-next] bonding: use rtnl_deref in bond_change_rx_flags() Veaceslav Falico @ 2014-07-16 9:05 ` Eric Dumazet 2014-07-16 9:34 ` Veaceslav Falico 0 siblings, 1 reply; 3+ messages in thread From: Eric Dumazet @ 2014-07-16 9:05 UTC (permalink / raw) To: Veaceslav Falico; +Cc: netdev, Jay Vosburgh, Andy Gospodarek On Wed, 2014-07-16 at 09:24 +0200, Veaceslav Falico wrote: > As it's always called with RTNL held, via dev_set_allmulti/promiscuity. > > CC: Jay Vosburgh <j.vosburgh@gmail.com> > CC: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Veaceslav Falico <vfalico@gmail.com> > --- > drivers/net/bonding/bond_main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index d643807..2998710 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -498,7 +498,7 @@ static int bond_set_promiscuity(struct bonding *bond, int inc) > int err = 0; > > if (bond_uses_primary(bond)) { > - struct slave *curr_active = bond_deref_active_protected(bond); > + struct slave *curr_active = rtnl_dereference(bond->curr_active_slave); > > /* write lock already acquired */ This seems dubious to me. What really protects curr_active_slave being modified ? Considering the presence of the previous comment, I assumed the sync point was the write lock. Not rtnl. If write lock is never held without rtnl, then maybe the write lock is useless, I don't know. But after your patch its not really consistent and we increase confusion. > if (curr_active) > @@ -524,7 +524,7 @@ static int bond_set_allmulti(struct bonding *bond, int inc) > int err = 0; > > if (bond_uses_primary(bond)) { > - struct slave *curr_active = bond_deref_active_protected(bond); > + struct slave *curr_active = rtnl_dereference(bond->curr_active_slave); > > /* write lock already acquired */ > if (curr_active) ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] bonding: use rtnl_deref in bond_change_rx_flags() 2014-07-16 9:05 ` Eric Dumazet @ 2014-07-16 9:34 ` Veaceslav Falico 0 siblings, 0 replies; 3+ messages in thread From: Veaceslav Falico @ 2014-07-16 9:34 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev, Jay Vosburgh, Andy Gospodarek On Wed, Jul 16, 2014 at 11:05:48AM +0200, Eric Dumazet wrote: >On Wed, 2014-07-16 at 09:24 +0200, Veaceslav Falico wrote: >> As it's always called with RTNL held, via dev_set_allmulti/promiscuity. >> >> CC: Jay Vosburgh <j.vosburgh@gmail.com> >> CC: Andy Gospodarek <andy@greyhouse.net> >> Signed-off-by: Veaceslav Falico <vfalico@gmail.com> >> --- >> drivers/net/bonding/bond_main.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index d643807..2998710 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -498,7 +498,7 @@ static int bond_set_promiscuity(struct bonding *bond, int inc) >> int err = 0; >> >> if (bond_uses_primary(bond)) { >> - struct slave *curr_active = bond_deref_active_protected(bond); >> + struct slave *curr_active = rtnl_dereference(bond->curr_active_slave); >> >> /* write lock already acquired */ > >This seems dubious to me. What really protects curr_active_slave being >modified ? That's indeed all messed up, also I assume you've meant "c_a_s variable changes its value" but not "the slave's internal parts are modified". > >Considering the presence of the previous comment, I assumed the sync >point was the write lock. Not rtnl. Good point, this comment should be removed. I'll send v2 if we'll agree with this one removed. > >If write lock is never held without rtnl, then maybe the write lock is >useless, I don't know. The c_a_s is set under rtnl, i.e. we can't have another slave there while we hold the rtnl. However, quite a few functions (alb usually) uses the c_a_s write lock to assure that the *current* slave isn't modified again (like - by re-entering this function) or in parallel with some another function, or even changing the current slave to which c_a_s points. i.e. rtnl prevents changing the slave to which c_a_s points, while the spinlock protects the interiors of the slave from changing, and the slave itself. So, the write lock isn't exactly useless currently, however it should go away. Yes, I know it's a mess. :( > >But after your patch its not really consistent and we increase >confusion. > >> if (curr_active) >> @@ -524,7 +524,7 @@ static int bond_set_allmulti(struct bonding *bond, int inc) >> int err = 0; >> >> if (bond_uses_primary(bond)) { >> - struct slave *curr_active = bond_deref_active_protected(bond); >> + struct slave *curr_active = rtnl_dereference(bond->curr_active_slave); >> >> /* write lock already acquired */ >> if (curr_active) > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-07-16 9:37 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-16 7:24 [PATCH net-next] bonding: use rtnl_deref in bond_change_rx_flags() Veaceslav Falico 2014-07-16 9:05 ` Eric Dumazet 2014-07-16 9:34 ` Veaceslav Falico
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).