netdev - Re: [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 09 Nov 2013 23:14:07 +0800
From:	Ding Tianhong <dthxman@...il.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
CC:	Ding Tianhong <dingtianhong@...wei.com>,
	Jay Vosburgh <fubar@...ibm.com>,
	Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>,
	Veaceslav Falico <vfalico@...hat.com>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb_monitor()

于 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@...ibm.com>
>>>> Suggested-by: Veaceslav Falico <vfalico@...hat.com>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@...wei.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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html