bonding: remove the no effect lock for bond_3ad_lacpdu_recv()

PATCH In review Ding Tianhong 3 participants 5 messages Sep 25, 2013 – Sep 26, 2013

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Ding Tianhong

author

2013-09-25 09:53 UTC #0 lore raw

PATCH v5 2/6 bonding: remove the no effect lock for bond_3ad_lacpdu_recv()

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 <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

---
 drivers/net/bonding/bond_3ad.c | 2 --
 1 file changed, 2 deletions(-)

drivers/net/bonding/bond_3ad.c +0 -2

index 7a3860f..c134f43 100644
@@ -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;
 }

Signature · 1 line

1.8.2.1

Veaceslav Falico

2013-09-25 10:35 UTC #1 lore raw

Quoted 2 lines · Ding Tianhong

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.

Quoted 30 lines

Signed-off-by Ding Tianhong <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

---
drivers/net/bonding/bond_3ad.c | 2 --

1 file changed, 2 deletions(-)

drivers/net/bonding/bond_3ad.c +0 -2

index 7a3860f..c134f43 100644
@@ -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;
 }

Signature · 8 lines

1.8.2.1



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

Ding Tianhong

author

2013-09-26 02:51 UTC #2 lore raw

Quoted 26 lines · Veaceslav Falico

Quoted 2 lines · Ding Tianhong
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.

Quoted 4 lines

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

Quoted 33 lines

Quoted 30 lines

Signed-off-by Ding Tianhong <[email protected]>

Cc Nikolay Aleksandrov <[email protected]>

---
drivers/net/bonding/bond_3ad.c | 2 --

1 file changed, 2 deletions(-)

drivers/net/bonding/bond_3ad.c +0 -2

index 7a3860f..c134f43 100644
@@ -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;
 }

Signature · 8 lines

1.8.2.1



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

Jay Vosburgh

2013-09-26 04:23 UTC #3 lore raw

Quoted 4 lines · Ding Tianhong

Quoted 3 lines · Veaceslav Falico
Quoted 2 lines · Ding Tianhong
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.

Quoted 38 lines

Quoted 22 lines
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.
Quoted 4 lines
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);
	}

Quoted 2 lines

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

Quoted 19 lines

Best regards
Ding Tianhong
Quoted 15 lines
Quoted 15 lines
Signed-off-by Ding Tianhong <[email protected]>
Cc Nikolay Aleksandrov <[email protected]>
---
drivers/net/bonding/bond_3ad.c | 2 --
1 file changed, 2 deletions(-)
drivers/net/bonding/bond_3ad.c +0 -0
index 7a3860f..c134f43 100644
@@ -2494,9 +2494,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
    if (!lacpdu)
        return ret;
 

Quoted 8 lines

drivers/net/bonding/bond_3ad.c +0 -2

-    read_lock(&bond->lock);
    ret = bond_3ad_rx_indication(lacpdu, slave, skb->len);
-    read_unlock(&bond->lock);
    return ret;
 }

Signature · 1 line

1.8.2.1

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]

Ding Tianhong

author

2013-09-26 06:19 UTC #4 lore raw

Quoted 12 lines · Jay Vosburgh

Quoted 4 lines · Ding Tianhong
Quoted 3 lines · Veaceslav Falico
Quoted 2 lines · Ding Tianhong
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.

Quoted 59 lines

Quoted 38 lines
Quoted 22 lines
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.
Quoted 4 lines
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);
	}
Quoted 2 lines
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;

Quoted 9 lines

[ 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.

Quoted 37 lines

Quoted 22 lines
Best regards
Ding Tianhong
Quoted 15 lines
Quoted 15 lines
Signed-off-by Ding Tianhong <[email protected]>
Cc Nikolay Aleksandrov <[email protected]>
---
drivers/net/bonding/bond_3ad.c | 2 --
1 file changed, 2 deletions(-)
drivers/net/bonding/bond_3ad.c +0 -0
index 7a3860f..c134f43 100644
@@ -2494,9 +2494,7 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
    if (!lacpdu)
        return ret;
 
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/net/bonding/bond_3ad.c +0 -2
-    read_lock(&bond->lock);
    ret = bond_3ad_rx_indication(lacpdu, slave, skb->len);
-    read_unlock(&bond->lock);
    return ret;
 }
 
Signature · 1 line
1.8.2.1
---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]
.