| 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
| ||
|
Message-ID: <527E46EA.5030804@redhat.com>
Date: Sat, 09 Nov 2013 15:30:02 +0100
From: Nikolay Aleksandrov <nikolay@...hat.com>
To: Ding Tianhong <dthxman@...il.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()
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
>>> @@ -1528,7 +1529,7 @@ void bond_alb_monitor(struct work_struct *work)
>>> read_lock(&bond->curr_slave_lock);
>>> - bond_for_each_slave(bond, slave, iter) {
>>> + bond_for_each_slave_rcu(bond, slave, iter) {
>>> tlb_clear_slave(bond, slave, 1);
>>> if (slave == bond->curr_active_slave) {
>>> SLAVE_TLB_INFO(slave).load =
>>> @@ -1552,11 +1553,9 @@ void bond_alb_monitor(struct work_struct *work)
>>> * dev_set_promiscuity requires rtnl and
>>> * nothing else. Avoid race with bond_close.
>>> */
>>> - read_unlock(&bond->lock);
>>> - if (!rtnl_trylock()) {
>>> - read_lock(&bond->lock);
>>> + rcu_read_unlock();
>>> + if (!rtnl_trylock())
>>> goto re_arm;
>>> - }
>>> bond_info->rlb_promisc_timeout_counter = 0;
>>> @@ -1568,7 +1567,7 @@ void bond_alb_monitor(struct work_struct *work)
>>> bond_info->primary_is_promisc = 0;
>>> rtnl_unlock();
>>> - read_lock(&bond->lock);
>>> + rcu_read_lock();
>>> }
>>> if (bond_info->rlb_rebalance) {
>>> @@ -1590,11 +1589,9 @@ void bond_alb_monitor(struct work_struct *work)
>>> }
>>> }
>>> }
>>> -
>>> + rcu_read_unlock();
>>> re_arm:
>>> queue_delayed_work(bond->wq, &bond->alb_work, alb_delta_in_ticks);
>>> -
>>> - read_unlock(&bond->lock);
>>> }
>>> /* assumption: called before the slave is attached to the bond
>>>
>> --
>> 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
>>
>
> --
> 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
--
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
Powered by blists - more mailing lists