* [PATCH net-next v5 2/6] bonding: remove the no effect lock for bond_3ad_lacpdu_recv()
@ 2013-09-25 9:52 Ding Tianhong
2013-09-25 10:33 ` Veaceslav Falico
0 siblings, 1 reply; 5+ messages in thread
From: Ding Tianhong @ 2013-09-25 9:52 UTC (permalink / raw)
To: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Veaceslav Falico, Netdev
There is no pointer needed read lock protection, remove the unnecessary lock
and improve performance for the 3ad recv path.
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Cc: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 7a3860f..c134f43 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2494,9 +2494,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
if (!lacpdu)
return ret;
- read_lock(&bond->lock);
ret = bond_3ad_rx_indication(lacpdu, slave, skb->len);
- read_unlock(&bond->lock);
return ret;
}
--
1.8.2.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v5 2/6] bonding: remove the no effect lock for bond_3ad_lacpdu_recv()
2013-09-25 9:52 [PATCH net-next v5 2/6] bonding: remove the no effect lock for bond_3ad_lacpdu_recv() Ding Tianhong
@ 2013-09-25 10:33 ` Veaceslav Falico
2013-09-26 2:51 ` Ding Tianhong
0 siblings, 1 reply; 5+ messages in thread
From: Veaceslav Falico @ 2013-09-25 10:33 UTC (permalink / raw)
To: Ding Tianhong
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
On Wed, Sep 25, 2013 at 05:52:19PM +0800, Ding Tianhong wrote:
>There is no pointer needed read lock protection, remove the unnecessary lock
>and improve performance for the 3ad recv path.
I don't really understand it. Here's the code path:
rx_handler (holding rcu_read_lock()) -> bond_handle_frame() ->
bond->recv_probe -> bond_3ad_lacpdu_recv(). So we're holding only the
rcu_read_lock() there. What stops us from racing with
bond_3ad_unbind_slave(), for example?
As in:
CPU0 CPU1
-------- -----------
... bond_3ad_unbind_slave()
bond_3ad_rx_indication() ...
if (!port->slave) { ... //slave is ok
port->slave = NULL;
ad_marker_info_received() ...
ad_marker_send() ...
slave = port->slave; ...
skb->dev = slave->dev; ...
^^^ NULL pointer dereference.
I'm not saying that this approach is wrong, maybe I'm missing something,
but when removing locks it's usually a good thing to do - to comment it in
depth in the commit message why it's not already needed.
>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>---
> drivers/net/bonding/bond_3ad.c | 2 --
> 1 file changed, 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index 7a3860f..c134f43 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -2494,9 +2494,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
> if (!lacpdu)
> return ret;
>
>- read_lock(&bond->lock);
> ret = bond_3ad_rx_indication(lacpdu, slave, skb->len);
>- read_unlock(&bond->lock);
> return ret;
> }
>
>--
>1.8.2.1
>
>
>
>--
>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 v5 2/6] bonding: remove the no effect lock for bond_3ad_lacpdu_recv()
2013-09-25 10:33 ` Veaceslav Falico
@ 2013-09-26 2:51 ` Ding Tianhong
2013-09-26 4:23 ` Jay Vosburgh
0 siblings, 1 reply; 5+ messages in thread
From: Ding Tianhong @ 2013-09-26 2:51 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
On 2013/9/25 18:33, Veaceslav Falico wrote:
> On Wed, Sep 25, 2013 at 05:52:19PM +0800, Ding Tianhong wrote:
>> There is no pointer needed read lock protection, remove the unnecessary lock
>> and improve performance for the 3ad recv path.
>
> I don't really understand it. Here's the code path:
>
> rx_handler (holding rcu_read_lock()) -> bond_handle_frame() ->
> bond->recv_probe -> bond_3ad_lacpdu_recv(). So we're holding only the
> rcu_read_lock() there. What stops us from racing with
> bond_3ad_unbind_slave(), for example?
>
> As in:
>
> CPU0 CPU1
> -------- -----------
> ... bond_3ad_unbind_slave()
> bond_3ad_rx_indication() ...
> if (!port->slave) { ... //slave is ok
> port->slave = NULL;
> ad_marker_info_received() ...
> ad_marker_send() ...
> slave = port->slave; ...
> skb->dev = slave->dev; ...
> ^^^ NULL pointer dereference.
>
>
> I'm not saying that this approach is wrong, maybe I'm missing something,
> but when removing locks it's usually a good thing to do - to comment it in
> depth in the commit message why it's not already needed.
>
no, it will not happend, pls review the cold:
netdev_rx_handler_unregister(slave_dev);
write_lock_bh(&bond->lock);
/* Inform AD package of unbinding of slave. */
if (bond->params.mode == BOND_MODE_8023AD) {
/* must be called before the slave is
* detached from the list
*/
bond_3ad_unbind_slave(slave);
}
netdev_rx_handler_unregiste() will remvoe the rx_handle before the bond_3ad_unbind_slave(),
it was safe to run bond_3ad_rx_indication().
Best regards
Ding Tianhong
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>> ---
>> drivers/net/bonding/bond_3ad.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index 7a3860f..c134f43 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -2494,9 +2494,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
>> if (!lacpdu)
>> return ret;
>>
>> - read_lock(&bond->lock);
>> ret = bond_3ad_rx_indication(lacpdu, slave, skb->len);
>> - read_unlock(&bond->lock);
>> return ret;
>> }
>>
>> --
>> 1.8.2.1
>>
>>
>>
>> --
>> 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 v5 2/6] bonding: remove the no effect lock for bond_3ad_lacpdu_recv()
2013-09-26 2:51 ` Ding Tianhong
@ 2013-09-26 4:23 ` Jay Vosburgh
2013-09-26 6:19 ` Ding Tianhong
0 siblings, 1 reply; 5+ messages in thread
From: Jay Vosburgh @ 2013-09-26 4:23 UTC (permalink / raw)
To: Ding Tianhong
Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
Ding Tianhong <dingtianhong@huawei.com> wrote:
>On 2013/9/25 18:33, Veaceslav Falico wrote:
>> On Wed, Sep 25, 2013 at 05:52:19PM +0800, Ding Tianhong wrote:
>>> There is no pointer needed read lock protection, remove the unnecessary lock
>>> and improve performance for the 3ad recv path.
How much does removing the lock around the LACPDU receive
processing improve performance? This is not high rate traffic; the
"fast" rate is one LACPDU per second (per port); the default rate is one
every 30 seconds.
>> I don't really understand it. Here's the code path:
>>
>> rx_handler (holding rcu_read_lock()) -> bond_handle_frame() ->
>> bond->recv_probe -> bond_3ad_lacpdu_recv(). So we're holding only the
>> rcu_read_lock() there. What stops us from racing with
>> bond_3ad_unbind_slave(), for example?
>>
>> As in:
>>
>> CPU0 CPU1
>> -------- -----------
>> ... bond_3ad_unbind_slave()
>> bond_3ad_rx_indication() ...
>> if (!port->slave) { ... //slave is ok
>> port->slave = NULL;
>> ad_marker_info_received() ...
>> ad_marker_send() ...
>> slave = port->slave; ...
>> skb->dev = slave->dev; ...
>> ^^^ NULL pointer dereference.
>>
>>
>> I'm not saying that this approach is wrong, maybe I'm missing something,
>> but when removing locks it's usually a good thing to do - to comment it in
>> depth in the commit message why it's not already needed.
>>
>
>no, it will not happend, pls review the cold:
> netdev_rx_handler_unregister(slave_dev);
> write_lock_bh(&bond->lock);
>
> /* Inform AD package of unbinding of slave. */
> if (bond->params.mode == BOND_MODE_8023AD) {
> /* must be called before the slave is
> * detached from the list
> */
> bond_3ad_unbind_slave(slave);
> }
>netdev_rx_handler_unregiste() will remvoe the rx_handle before the bond_3ad_unbind_slave(),
>it was safe to run bond_3ad_rx_indication().
I'm not sure this is safe if bond_3ad_rx_indication is started
prior to the unbind, e.g.,
CPU 0 CPU 1
---- -----
bond_3ad_rx_indication
[ pass port->slave test ]
[ ... ] rx_handler_unregister
[ state machine lock could be
contended, forcing us to wait ]
__get_state_machine_lock
write_lock(&bond->lock)
bond_3ad_unbind_slave()
[ ... ]
port->slave = NULL;
[ got the lock ]
ad_rx_machine(lacpdu, port)
[ detect loopback ]
pr_err(... port->slave->bond->dev->name)
or that ad_marker case that Veaceslav describes.
-J
>Best regards
>Ding Tianhong
>
>
>>>
>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>> Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>>> ---
>>> drivers/net/bonding/bond_3ad.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>> index 7a3860f..c134f43 100644
>>> --- a/drivers/net/bonding/bond_3ad.c
>>> +++ b/drivers/net/bonding/bond_3ad.c
>>> @@ -2494,9 +2494,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
>>> if (!lacpdu)
>>> return ret;
>>>
>>> - read_lock(&bond->lock);
>>> ret = bond_3ad_rx_indication(lacpdu, slave, skb->len);
>>> - read_unlock(&bond->lock);
>>> return ret;
>>> }
>>>
>>> --
>>> 1.8.2.1
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v5 2/6] bonding: remove the no effect lock for bond_3ad_lacpdu_recv()
2013-09-26 4:23 ` Jay Vosburgh
@ 2013-09-26 6:19 ` Ding Tianhong
0 siblings, 0 replies; 5+ messages in thread
From: Ding Tianhong @ 2013-09-26 6:19 UTC (permalink / raw)
To: Jay Vosburgh
Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
On 2013/9/26 12:23, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@huawei.com> wrote:
>
>> On 2013/9/25 18:33, Veaceslav Falico wrote:
>>> On Wed, Sep 25, 2013 at 05:52:19PM +0800, Ding Tianhong wrote:
>>>> There is no pointer needed read lock protection, remove the unnecessary lock
>>>> and improve performance for the 3ad recv path.
>
> How much does removing the lock around the LACPDU receive
> processing improve performance? This is not high rate traffic; the
> "fast" rate is one LACPDU per second (per port); the default rate is one
> every 30 seconds.
>
agree.
>>> I don't really understand it. Here's the code path:
>>>
>>> rx_handler (holding rcu_read_lock()) -> bond_handle_frame() ->
>>> bond->recv_probe -> bond_3ad_lacpdu_recv(). So we're holding only the
>>> rcu_read_lock() there. What stops us from racing with
>>> bond_3ad_unbind_slave(), for example?
>>>
>>> As in:
>>>
>>> CPU0 CPU1
>>> -------- -----------
>>> ... bond_3ad_unbind_slave()
>>> bond_3ad_rx_indication() ...
>>> if (!port->slave) { ... //slave is ok
>>> port->slave = NULL;
>>> ad_marker_info_received() ...
>>> ad_marker_send() ...
>>> slave = port->slave; ...
>>> skb->dev = slave->dev; ...
>>> ^^^ NULL pointer dereference.
>>>
>>>
>>> I'm not saying that this approach is wrong, maybe I'm missing something,
>>> but when removing locks it's usually a good thing to do - to comment it in
>>> depth in the commit message why it's not already needed.
>>>
>>
>> no, it will not happend, pls review the cold:
>> netdev_rx_handler_unregister(slave_dev);
>> write_lock_bh(&bond->lock);
>>
>> /* Inform AD package of unbinding of slave. */
>> if (bond->params.mode == BOND_MODE_8023AD) {
>> /* must be called before the slave is
>> * detached from the list
>> */
>> bond_3ad_unbind_slave(slave);
>> }
>> netdev_rx_handler_unregiste() will remvoe the rx_handle before the bond_3ad_unbind_slave(),
>> it was safe to run bond_3ad_rx_indication().
>
> I'm not sure this is safe if bond_3ad_rx_indication is started
> prior to the unbind, e.g.,
>
> CPU 0 CPU 1
> ---- -----
> bond_3ad_rx_indication
> [ pass port->slave test ]
> [ ... ] rx_handler_unregister
>
> [ state machine lock could be
> contended, forcing us to wait ]
> __get_state_machine_lock
>
> write_lock(&bond->lock)
> bond_3ad_unbind_slave()
> [ ... ]
> port->slave = NULL;
>
> [ got the lock ]
> ad_rx_machine(lacpdu, port)
> [ detect loopback ]
> pr_err(... port->slave->bond->dev->name)
>
> or that ad_marker case that Veaceslav describes.
>
> -J
>
yes, I miss one thing here, there is no rcu_read_lock() here, so when enter
bond_3ad_unbind_slave(), bond_3ad_rx_indication() was running.
we should miss the patch, thanks.
>> Best regards
>> Ding Tianhong
>>
>>
>>>>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>>> Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>>>> ---
>>>> drivers/net/bonding/bond_3ad.c | 2 --
>>>> 1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>>>> index 7a3860f..c134f43 100644
>>>> --- a/drivers/net/bonding/bond_3ad.c
>>>> +++ b/drivers/net/bonding/bond_3ad.c
>>>> @@ -2494,9 +2494,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
>>>> if (!lacpdu)
>>>> return ret;
>>>>
>>>> - read_lock(&bond->lock);
>>>> ret = bond_3ad_rx_indication(lacpdu, slave, skb->len);
>>>> - read_unlock(&bond->lock);
>>>> return ret;
>>>> }
>>>>
>>>> --
>>>> 1.8.2.1
>
> ---
> -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>
>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-26 6:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-25 9:52 [PATCH net-next v5 2/6] bonding: remove the no effect lock for bond_3ad_lacpdu_recv() Ding Tianhong
2013-09-25 10:33 ` Veaceslav Falico
2013-09-26 2:51 ` Ding Tianhong
2013-09-26 4:23 ` Jay Vosburgh
2013-09-26 6:19 ` 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).