Re: [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb_monitor()

[Ding Tianhong <dthxman@gmail.com>]

于 2013/11/9 22:30, Nikolay Aleksandrov 写道:
> On 11/09/2013 03:15 PM, Ding Tianhong wrote:
>> 于 2013/11/9 0:07, Nikolay Aleksandrov 写道:
>>> On 11/08/2013 03:07 AM, Ding Tianhong wrote:
>>>> The bond_alb_monitor use bond lock to protect the bond slave list,
>>>> it is no effect here, we need to use RTNL or RCU to replace bond lock,
>>>> the bond_alb_monitor will called 10 times one second, RTNL may loss
>>>> performance here, so the bond lock replace with RCU to protect the
>>>> bond slave list, also the RTNL is preserved, the logic of the monitor
>>>> did not changed.
>>>>
>>>> Suggested-by: Jay Vosburgh <fubar@us.ibm.com>
>>>> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>>> ---
>>>>    drivers/net/bonding/bond_alb.c | 21 +++++++++------------
>>>>    1 file changed, 9 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/bond_alb.c
>>>> b/drivers/net/bonding/bond_alb.c
>>>> index 1fae915..ffdb91b 100644
>>>> --- a/drivers/net/bonding/bond_alb.c
>>>> +++ b/drivers/net/bonding/bond_alb.c
>>>> @@ -816,7 +816,7 @@ static void rlb_rebalance(struct bonding *bond)
>>>>        for (; hash_index != RLB_NULL_INDEX;
>>>>             hash_index = client_info->used_next) {
>>>>            client_info = &(bond_info->rx_hashtbl[hash_index]);
>>>> -        assigned_slave = rlb_next_rx_slave(bond);
>>>> +        assigned_slave = __rlb_next_rx_slave(bond);
>>>>            if (assigned_slave && (client_info->slave != assigned_slave)) {
>>>>                client_info->slave = assigned_slave;
>>>>                client_info->ntt = 1;
>>>> @@ -1495,9 +1495,10 @@ void bond_alb_monitor(struct work_struct *work)
>>>>        struct list_head *iter;
>>>>        struct slave *slave;
>>>>    -    read_lock(&bond->lock);
>>>> +    rcu_read_lock();
>>>>    -    if (!bond_has_slaves(bond)) {
>>>> +    if (!bond_has_slaves_rcu(bond)) {
>>>> +        rcu_read_unlock();
>>>>            bond_info->tx_rebalance_counter = 0;
>>>>            bond_info->lp_counter = 0;
>>>>            goto re_arm;
>>> If I'm not mistaken there's one more bond_for_each_slave() inside this
>>> function
>>> which should be converted to RCU.
>> But I really could not find any place should converted to RCU,
>>
>> __rlb_next_rx_slave() is in RCU yet.
>>
>> pls remind me if I miss something.
>>
>> Regards
>> Ding
>>
>>
> I was talking about this piece of code inside bond_alb_monitor():
>          /* send learning packets */
>          if (bond_info->lp_counter >= BOND_ALB_LP_TICKS(bond)) {
>                  /* change of curr_active_slave involves swapping of mac
> addresses.
>                   * in order to avoid this swapping from happening while
>                   * sending the learning packets, the curr_slave_lock must
> be held for
>                   * read.
>                   */
>                  read_lock(&bond->curr_slave_lock);
>
>                  bond_for_each_slave(bond, slave, iter)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                          alb_send_learning_packets(slave, slave->dev->dev_addr);
>
>                  read_unlock(&bond->curr_slave_lock);
>
>                  bond_info->lp_counter = 0;
>          }
>
> This is copied after your patch-set was applied.
>
> Cheers,
>   Nik
oh, yes, thanks, I take the focus on the wrong place.

Regards
Ding