* [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb_monitor()
@ 2013-11-08 2:07 Ding Tianhong
2013-11-08 16:07 ` Nikolay Aleksandrov
0 siblings, 1 reply; 5+ messages in thread
From: Ding Tianhong @ 2013-11-08 2:07 UTC (permalink / raw)
To: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Veaceslav Falico, Netdev
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;
@@ -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
--
1.8.2.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb_monitor()
2013-11-08 2:07 [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb_monitor() Ding Tianhong
@ 2013-11-08 16:07 ` Nikolay Aleksandrov
2013-11-09 14:15 ` Ding Tianhong
0 siblings, 1 reply; 5+ messages in thread
From: Nikolay Aleksandrov @ 2013-11-08 16:07 UTC (permalink / raw)
To: Ding Tianhong
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Veaceslav Falico,
Netdev
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.
> @@ -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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb_monitor()
2013-11-08 16:07 ` Nikolay Aleksandrov
@ 2013-11-09 14:15 ` Ding Tianhong
2013-11-09 14:30 ` Nikolay Aleksandrov
0 siblings, 1 reply; 5+ messages in thread
From: Ding Tianhong @ 2013-11-09 14:15 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Veaceslav Falico, Netdev
于 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
>> @@ -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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb_monitor()
2013-11-09 14:15 ` Ding Tianhong
@ 2013-11-09 14:30 ` Nikolay Aleksandrov
2013-11-09 15:14 ` Ding Tianhong
0 siblings, 1 reply; 5+ messages in thread
From: Nikolay Aleksandrov @ 2013-11-09 14:30 UTC (permalink / raw)
To: Ding Tianhong
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Veaceslav Falico, Netdev
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
>>> @@ -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@vger.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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb_monitor()
2013-11-09 14:30 ` Nikolay Aleksandrov
@ 2013-11-09 15:14 ` Ding Tianhong
0 siblings, 0 replies; 5+ messages in thread
From: Ding Tianhong @ 2013-11-09 15:14 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Veaceslav Falico, Netdev
于 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-11-09 15:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-08 2:07 [PATCH net-next v2 3/10] bonding: rebuild the lock use for bond_alb_monitor() Ding Tianhong
2013-11-08 16:07 ` Nikolay Aleksandrov
2013-11-09 14:15 ` Ding Tianhong
2013-11-09 14:30 ` Nikolay Aleksandrov
2013-11-09 15:14 ` Ding Tianhong
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).