[PATCH net-next v2 0/10] bonding: rebuild the lock use for bond monitor

[PATCH net-next v2 0/10] bonding: rebuild the lock use for bond monitor

[Ding Tianhong]

Now the bond slave list is not protected by bond lock, only by RTNL,
but the monitor still use the bond lock to protect the slave list,
it is useless, according to the Veaceslav's opinion, there were
three way to fix the protect problem:

1. add bond_master_upper_dev_link() and bond_upper_dev_unlink()
   in bond->lock, but it is unsafe to call call_netdevice_notifiers()
   in write lock.
2. remove unused bond->lock for monitor function, only use the exist
   rtnl lock(), it will take performance loss in fast path.
3. use RCU to protect the slave list, of course, performance is better,
   but in slow path, it is ignored.

obviously the solution 1 is not fit here, I will consider the 2 and 3
solution. My principle is simple, if in fast path, RCU is better,
otherwise in slow path, both is well, but according to the Jay Vosburgh's
opinion, the monitor will loss performace if use RTNL to protect the all
slave list, so remove the bond lock and replace with RCU.

The second problem is the curr_slave_lock for bond, it is too old and
unwanted in many place, because the curr_active_slave would only be
changed in 3 place:

1. enslave slave.
2. release slave.
3. change active slave.

all above were already holding bond lock, RTNL and curr_slave_lock
together, it is tedious and no need to add so mach lock, when change
the curr_active_slave, you have to hold the RTNL and curr_slave_lock
together, and when you read the curr_active_slave, RTNL or curr_slave_lock,
any one of them is no problem.

for the stability, I did not change the logic for the monitor,
all change is clear and simple, I have test the patch set for lockdep,
it work well and stability.

v2. accept the Jay Vosburgh's opinion, remove the RTNL and replace with RCU,
    also add some rcu function for bond use, so the patch set reach 10.

Best Regards
Ding Tianhong

Ding Tianhong (10):
  bonding: remove the no effect lock for bond_select_active_slave()
  bonding: rebuild the lock use for bond_mii_monitor()
  bonding: rebuild the lock use for bond_alb_monitor()
  bonding: rebuild the lock use for bond_loadbalance_arp_mon()
  net: add and export netdev_adjacent_get_private_rcu()
  bonding: rebuild the lock use for bond_activebackup_arp_mon()
  bonding: rebuild the lock use for bond_3ad_state_machine_handler()
  bonding: remove unwanted lock for bond_option_active_slave_set()
  bonding: remvoe unwanted lock for bond enslave and release
  bonding: remove unwanted lock for bond_store_primaryxxx()

 drivers/net/bonding/bond_3ad.c     |  21 +++---
 drivers/net/bonding/bond_alb.c     |  33 ++++-----
 drivers/net/bonding/bond_main.c    | 137 +++++++++++++++++--------------------
 drivers/net/bonding/bond_options.c |   2 -
 drivers/net/bonding/bond_sysfs.c   |   4 --
 drivers/net/bonding/bonding.h      |  13 ++++
 include/linux/netdevice.h          |   1 +
 net/core/dev.c                     |  10 +++
 8 files changed, 108 insertions(+), 113 deletions(-)

-- 
1.8.2.1

Re: [PATCH net-next v2 0/10] bonding: rebuild the lock use for bond monitor

[David Miller]

Such patches should not be submitted at this time, the merge window
has openned up and therefore the net-next tree is closed.

Please wait for the merge window to close and the net-next tree
to open back up before submitting these changes.

Thanks.

Re: [PATCH net-next v2 0/10] bonding: rebuild the lock use for bond monitor

[Ben Hutchings]

On Fri, 2013-11-08 at 01:45 -0500, David Miller wrote:
> Such patches should not be submitted at this time, the merge window
> has openned up and therefore the net-next tree is closed.
> 
> Please wait for the merge window to close and the net-next tree
> to open back up before submitting these changes.

These look like bug fixes that should have been submitted for the net
tree.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next v2 0/10] bonding: rebuild the lock use for bond monitor

[Veaceslav Falico]

On Tue, Nov 12, 2013 at 11:26:27PM +0000, Ben Hutchings wrote:
>On Fri, 2013-11-08 at 01:45 -0500, David Miller wrote:
>> Such patches should not be submitted at this time, the merge window
>> has openned up and therefore the net-next tree is closed.
>>
>> Please wait for the merge window to close and the net-next tree
>> to open back up before submitting these changes.
>
>These look like bug fixes that should have been submitted for the net
>tree.

I am not really sure that it's suitable for net tree, cause these aren't
really bugfixes - but rather locking removal. It might fix/improve some
situations - but the changes touch really sensitive parts and I think they
should go through net-next, to brew a little.

>
>Ben.
>
>-- 
>Ben Hutchings, Staff Engineer, Solarflare
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.
>

Re: [PATCH net-next v2 0/10] bonding: rebuild the lock use for bond monitor

[Nikolay Aleksandrov]

On 11/08/2013 03:07 AM, Ding Tianhong wrote:
> Now the bond slave list is not protected by bond lock, only by RTNL,
> but the monitor still use the bond lock to protect the slave list,
> it is useless, according to the Veaceslav's opinion, there were
> three way to fix the protect problem:
> 
> 1. add bond_master_upper_dev_link() and bond_upper_dev_unlink()
>    in bond->lock, but it is unsafe to call call_netdevice_notifiers()
>    in write lock.
> 2. remove unused bond->lock for monitor function, only use the exist
>    rtnl lock(), it will take performance loss in fast path.
> 3. use RCU to protect the slave list, of course, performance is better,
>    but in slow path, it is ignored.
> 
> obviously the solution 1 is not fit here, I will consider the 2 and 3
> solution. My principle is simple, if in fast path, RCU is better,
> otherwise in slow path, both is well, but according to the Jay Vosburgh's
> opinion, the monitor will loss performace if use RTNL to protect the all
> slave list, so remove the bond lock and replace with RCU.
> 
> The second problem is the curr_slave_lock for bond, it is too old and
> unwanted in many place, because the curr_active_slave would only be
> changed in 3 place:
> 
> 1. enslave slave.
> 2. release slave.
> 3. change active slave.
> 
> all above were already holding bond lock, RTNL and curr_slave_lock
> together, it is tedious and no need to add so mach lock, when change
> the curr_active_slave, you have to hold the RTNL and curr_slave_lock
> together, and when you read the curr_active_slave, RTNL or curr_slave_lock,
> any one of them is no problem.
> 
> for the stability, I did not change the logic for the monitor,
> all change is clear and simple, I have test the patch set for lockdep,
> it work well and stability.
> 
> v2. accept the Jay Vosburgh's opinion, remove the RTNL and replace with RCU,
>     also add some rcu function for bond use, so the patch set reach 10.
> 
> Best Regards
> Ding Tianhong
> 

Hi,
I've left my comments from a quick overview of the patches, my opinion on the
patchset is that it wasn't tested thoroughly enough (or at all). There're
multiple places that use a weaker compiler barrier instead of directly using
rcu_dereference() or rcu_access_pointer(), also there're multiple places which
can directly use macros already present in the RCU API.

Cheers,
 Nik

Re: [PATCH net-next v2 0/10] bonding: rebuild the lock use for bond monitor

[Ding Tianhong]

于 2013/11/9 0:32, Nikolay Aleksandrov 写道:
> On 11/08/2013 03:07 AM, Ding Tianhong wrote:
>> Now the bond slave list is not protected by bond lock, only by RTNL,
>> but the monitor still use the bond lock to protect the slave list,
>> it is useless, according to the Veaceslav's opinion, there were
>> three way to fix the protect problem:
>>
>> 1. add bond_master_upper_dev_link() and bond_upper_dev_unlink()
>>     in bond->lock, but it is unsafe to call call_netdevice_notifiers()
>>     in write lock.
>> 2. remove unused bond->lock for monitor function, only use the exist
>>     rtnl lock(), it will take performance loss in fast path.
>> 3. use RCU to protect the slave list, of course, performance is better,
>>     but in slow path, it is ignored.
>>
>> obviously the solution 1 is not fit here, I will consider the 2 and 3
>> solution. My principle is simple, if in fast path, RCU is better,
>> otherwise in slow path, both is well, but according to the Jay Vosburgh's
>> opinion, the monitor will loss performace if use RTNL to protect the all
>> slave list, so remove the bond lock and replace with RCU.
>>
>> The second problem is the curr_slave_lock for bond, it is too old and
>> unwanted in many place, because the curr_active_slave would only be
>> changed in 3 place:
>>
>> 1. enslave slave.
>> 2. release slave.
>> 3. change active slave.
>>
>> all above were already holding bond lock, RTNL and curr_slave_lock
>> together, it is tedious and no need to add so mach lock, when change
>> the curr_active_slave, you have to hold the RTNL and curr_slave_lock
>> together, and when you read the curr_active_slave, RTNL or curr_slave_lock,
>> any one of them is no problem.
>>
>> for the stability, I did not change the logic for the monitor,
>> all change is clear and simple, I have test the patch set for lockdep,
>> it work well and stability.
>>
>> v2. accept the Jay Vosburgh's opinion, remove the RTNL and replace with RCU,
>>      also add some rcu function for bond use, so the patch set reach 10.
>>
>> Best Regards
>> Ding Tianhong
>>
> Hi,
> I've left my comments from a quick overview of the patches, my opinion on the
> patchset is that it wasn't tested thoroughly enough (or at all). There're
> multiple places that use a weaker compiler barrier instead of directly using
> rcu_dereference() or rcu_access_pointer(), also there're multiple places which
> can directly use macros already present in the RCU API.
>
> Cheers,
>   Nik

Thanks, Nik, for overview the long patches, point out the problem, I 
will review the
details which your point out and fix it.

Best Regards
Ding

>
> --
> 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
>