* [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).