* [PATCH net-next 2/4] bonding: use RCU protection for alb xmit path
@ 2013-09-29 11:45 Ding Tianhong
2013-09-30 10:52 ` Veaceslav Falico
0 siblings, 1 reply; 3+ messages in thread
From: Ding Tianhong @ 2013-09-29 11:45 UTC (permalink / raw)
To: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Veaceslav Falico, Netdev
The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
(bonding: initial RCU conversion) has convert the roundrobin,
active-backup, broadcast and xor xmit path to rcu protection,
the performance will be better for these mode, so this time,
convert xmit path for alb mode.
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
Cc: Nikolay Aleksandrov <nikolay@redhat.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_alb.c | 23 +++++++++--------------
drivers/net/bonding/bonding.h | 2 +-
2 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index e960418..a1444d5 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -230,7 +230,7 @@ static struct slave *tlb_get_least_loaded_slave(struct bonding *bond)
max_gap = LLONG_MIN;
/* Find the slave with the largest gap */
- bond_for_each_slave(bond, slave, iter) {
+ bond_for_each_slave_rcu(bond, slave, iter) {
if (SLAVE_IS_OK(slave)) {
long long gap = compute_gap(slave);
@@ -387,7 +387,7 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
struct list_head *iter;
bool found = false;
- bond_for_each_slave(bond, slave, iter) {
+ bond_for_each_slave_rcu(bond, slave, iter) {
if (!SLAVE_IS_OK(slave))
continue;
if (!found) {
@@ -628,12 +628,14 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
{
struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
struct arp_pkt *arp = arp_pkt(skb);
- struct slave *assigned_slave;
+ struct slave *assigned_slave, *curr_active_slave;
struct rlb_client_info *client_info;
u32 hash_index = 0;
_lock_rx_hashtbl(bond);
+ curr_active_slave = rcu_dereference(bond->curr_active_slave);
+
hash_index = _simple_hash((u8 *)&arp->ip_dst, sizeof(arp->ip_dst));
client_info = &(bond_info->rx_hashtbl[hash_index]);
@@ -658,8 +660,8 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
* that the new client can be assigned to this entry.
*/
if (bond->curr_active_slave &&
- client_info->slave != bond->curr_active_slave) {
- client_info->slave = bond->curr_active_slave;
+ client_info->slave != curr_active_slave) {
+ client_info->slave = curr_active_slave;
rlb_update_client(client_info);
}
}
@@ -1343,11 +1345,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
skb_reset_mac_header(skb);
eth_data = eth_hdr(skb);
- /* make sure that the curr_active_slave do not change during tx
- */
- read_lock(&bond->lock);
- read_lock(&bond->curr_slave_lock);
-
switch (ntohs(skb->protocol)) {
case ETH_P_IP: {
const struct iphdr *iph = ip_hdr(skb);
@@ -1429,12 +1426,12 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
if (!tx_slave) {
/* unbalanced or unassigned, send through primary */
- tx_slave = bond->curr_active_slave;
+ tx_slave = rcu_dereference(bond->curr_active_slave);
bond_info->unbalanced_load += skb->len;
}
if (tx_slave && SLAVE_IS_OK(tx_slave)) {
- if (tx_slave != bond->curr_active_slave) {
+ if (tx_slave != rcu_dereference(bond->curr_active_slave)) {
memcpy(eth_data->h_source,
tx_slave->dev->dev_addr,
ETH_ALEN);
@@ -1449,8 +1446,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
}
}
- read_unlock(&bond->curr_slave_lock);
- read_unlock(&bond->lock);
if (res) {
/* no suitable interface, frame not sent */
kfree_skb(skb);
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 713b6af..1c7d559 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -459,7 +459,7 @@ static inline struct slave *bond_slave_has_mac(struct bonding *bond,
struct list_head *iter;
struct slave *tmp;
- bond_for_each_slave(bond, tmp, iter)
+ bond_for_each_slave_rcu(bond, tmp, iter)
if (ether_addr_equal_64bits(mac, tmp->dev->dev_addr))
return tmp;
--
1.8.2.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next 2/4] bonding: use RCU protection for alb xmit path
2013-09-29 11:45 [PATCH net-next 2/4] bonding: use RCU protection for alb xmit path Ding Tianhong
@ 2013-09-30 10:52 ` Veaceslav Falico
2013-09-30 11:19 ` Ding Tianhong
0 siblings, 1 reply; 3+ messages in thread
From: Veaceslav Falico @ 2013-09-30 10:52 UTC (permalink / raw)
To: Ding Tianhong
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
On Sun, Sep 29, 2013 at 07:45:05PM +0800, Ding Tianhong wrote:
>The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
>(bonding: initial RCU conversion) has convert the roundrobin,
>active-backup, broadcast and xor xmit path to rcu protection,
>the performance will be better for these mode, so this time,
>convert xmit path for alb mode.
>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>
>---
> drivers/net/bonding/bond_alb.c | 23 +++++++++--------------
> drivers/net/bonding/bonding.h | 2 +-
> 2 files changed, 10 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index e960418..a1444d5 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -230,7 +230,7 @@ static struct slave *tlb_get_least_loaded_slave(struct bonding *bond)
> max_gap = LLONG_MIN;
>
> /* Find the slave with the largest gap */
>- bond_for_each_slave(bond, slave, iter) {
>+ bond_for_each_slave_rcu(bond, slave, iter) {
> if (SLAVE_IS_OK(slave)) {
> long long gap = compute_gap(slave);
>
>@@ -387,7 +387,7 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
> struct list_head *iter;
> bool found = false;
>
>- bond_for_each_slave(bond, slave, iter) {
>+ bond_for_each_slave_rcu(bond, slave, iter) {
> if (!SLAVE_IS_OK(slave))
> continue;
> if (!found) {
>@@ -628,12 +628,14 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
> {
> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> struct arp_pkt *arp = arp_pkt(skb);
>- struct slave *assigned_slave;
>+ struct slave *assigned_slave, *curr_active_slave;
> struct rlb_client_info *client_info;
> u32 hash_index = 0;
>
> _lock_rx_hashtbl(bond);
>
>+ curr_active_slave = rcu_dereference(bond->curr_active_slave);
>+
> hash_index = _simple_hash((u8 *)&arp->ip_dst, sizeof(arp->ip_dst));
> client_info = &(bond_info->rx_hashtbl[hash_index]);
>
>@@ -658,8 +660,8 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
> * that the new client can be assigned to this entry.
> */
> if (bond->curr_active_slave &&
>- client_info->slave != bond->curr_active_slave) {
>- client_info->slave = bond->curr_active_slave;
>+ client_info->slave != curr_active_slave) {
>+ client_info->slave = curr_active_slave;
> rlb_update_client(client_info);
> }
> }
>@@ -1343,11 +1345,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> skb_reset_mac_header(skb);
> eth_data = eth_hdr(skb);
>
>- /* make sure that the curr_active_slave do not change during tx
>- */
>- read_lock(&bond->lock);
>- read_lock(&bond->curr_slave_lock);
>-
> switch (ntohs(skb->protocol)) {
> case ETH_P_IP: {
> const struct iphdr *iph = ip_hdr(skb);
>@@ -1429,12 +1426,12 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>
> if (!tx_slave) {
> /* unbalanced or unassigned, send through primary */
>- tx_slave = bond->curr_active_slave;
>+ tx_slave = rcu_dereference(bond->curr_active_slave);
> bond_info->unbalanced_load += skb->len;
> }
>
> if (tx_slave && SLAVE_IS_OK(tx_slave)) {
>- if (tx_slave != bond->curr_active_slave) {
>+ if (tx_slave != rcu_dereference(bond->curr_active_slave)) {
> memcpy(eth_data->h_source,
> tx_slave->dev->dev_addr,
> ETH_ALEN);
>@@ -1449,8 +1446,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
> }
> }
>
>- read_unlock(&bond->curr_slave_lock);
>- read_unlock(&bond->lock);
> if (res) {
> /* no suitable interface, frame not sent */
> kfree_skb(skb);
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index 713b6af..1c7d559 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -459,7 +459,7 @@ static inline struct slave *bond_slave_has_mac(struct bonding *bond,
> struct list_head *iter;
> struct slave *tmp;
>
>- bond_for_each_slave(bond, tmp, iter)
>+ bond_for_each_slave_rcu(bond, tmp, iter)
This suggests that rcu_read_lock() is always held here, however it's not
the case, as far as I can tell:
bond_release()/bond_uninit() ->
__bond_release_one() ->
bond_alb_deinit_slave() ->
alb_change_hw_addr_on_detach() ->
bond_slave_has_mac()
There is nobody that takes rcu_read_lock() there. And, for example, when
doing:
echo -$slave > /sys/class/net/$bond_int/bonding/slaves
nobody is holding it. And, btw, in bond_for_each_slave_rcu(), which is
actually a wrapper for netdev_for_each_lower_private_rcu(), which calls
netdev_lower_get_next_private_rcu(), which has:
4543 void *netdev_lower_get_next_private_rcu(struct net_device *dev,
4544 struct list_head **iter)
4545 {
4546 struct netdev_adjacent *lower;
4547
4548 WARN_ON_ONCE(!rcu_read_lock_held());
so you should have seen that warning when trying to remove a slave in alb
mode.
> if (ether_addr_equal_64bits(mac, tmp->dev->dev_addr))
> return tmp;
>
>--
>1.8.2.1
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next 2/4] bonding: use RCU protection for alb xmit path
2013-09-30 10:52 ` Veaceslav Falico
@ 2013-09-30 11:19 ` Ding Tianhong
0 siblings, 0 replies; 3+ messages in thread
From: Ding Tianhong @ 2013-09-30 11:19 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
On 2013/9/30 18:52, Veaceslav Falico wrote:
> On Sun, Sep 29, 2013 at 07:45:05PM +0800, Ding Tianhong wrote:
>> The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
>> (bonding: initial RCU conversion) has convert the roundrobin,
>> active-backup, broadcast and xor xmit path to rcu protection,
>> the performance will be better for these mode, so this time,
>> convert xmit path for alb mode.
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> Cc: Nikolay Aleksandrov <nikolay@redhat.com>
>> Cc: Veaceslav Falico <vfalico@redhat.com>
>> ---
>> drivers/net/bonding/bond_alb.c | 23 +++++++++--------------
>> drivers/net/bonding/bonding.h | 2 +-
>> 2 files changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index e960418..a1444d5 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -230,7 +230,7 @@ static struct slave *tlb_get_least_loaded_slave(struct bonding *bond)
>> max_gap = LLONG_MIN;
>>
>> /* Find the slave with the largest gap */
>> - bond_for_each_slave(bond, slave, iter) {
>> + bond_for_each_slave_rcu(bond, slave, iter) {
>> if (SLAVE_IS_OK(slave)) {
>> long long gap = compute_gap(slave);
>>
>> @@ -387,7 +387,7 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
>> struct list_head *iter;
>> bool found = false;
>>
>> - bond_for_each_slave(bond, slave, iter) {
>> + bond_for_each_slave_rcu(bond, slave, iter) {
>> if (!SLAVE_IS_OK(slave))
>> continue;
>> if (!found) {
>> @@ -628,12 +628,14 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
>> {
>> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> struct arp_pkt *arp = arp_pkt(skb);
>> - struct slave *assigned_slave;
>> + struct slave *assigned_slave, *curr_active_slave;
>> struct rlb_client_info *client_info;
>> u32 hash_index = 0;
>>
>> _lock_rx_hashtbl(bond);
>>
>> + curr_active_slave = rcu_dereference(bond->curr_active_slave);
>> +
>> hash_index = _simple_hash((u8 *)&arp->ip_dst, sizeof(arp->ip_dst));
>> client_info = &(bond_info->rx_hashtbl[hash_index]);
>>
>> @@ -658,8 +660,8 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
>> * that the new client can be assigned to this entry.
>> */
>> if (bond->curr_active_slave &&
>> - client_info->slave != bond->curr_active_slave) {
>> - client_info->slave = bond->curr_active_slave;
>> + client_info->slave != curr_active_slave) {
>> + client_info->slave = curr_active_slave;
>> rlb_update_client(client_info);
>> }
>> }
>> @@ -1343,11 +1345,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>> skb_reset_mac_header(skb);
>> eth_data = eth_hdr(skb);
>>
>> - /* make sure that the curr_active_slave do not change during tx
>> - */
>> - read_lock(&bond->lock);
>> - read_lock(&bond->curr_slave_lock);
>> -
>> switch (ntohs(skb->protocol)) {
>> case ETH_P_IP: {
>> const struct iphdr *iph = ip_hdr(skb);
>> @@ -1429,12 +1426,12 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>>
>> if (!tx_slave) {
>> /* unbalanced or unassigned, send through primary */
>> - tx_slave = bond->curr_active_slave;
>> + tx_slave = rcu_dereference(bond->curr_active_slave);
>> bond_info->unbalanced_load += skb->len;
>> }
>>
>> if (tx_slave && SLAVE_IS_OK(tx_slave)) {
>> - if (tx_slave != bond->curr_active_slave) {
>> + if (tx_slave != rcu_dereference(bond->curr_active_slave)) {
>> memcpy(eth_data->h_source,
>> tx_slave->dev->dev_addr,
>> ETH_ALEN);
>> @@ -1449,8 +1446,6 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>> }
>> }
>>
>> - read_unlock(&bond->curr_slave_lock);
>> - read_unlock(&bond->lock);
>> if (res) {
>> /* no suitable interface, frame not sent */
>> kfree_skb(skb);
>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>> index 713b6af..1c7d559 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -459,7 +459,7 @@ static inline struct slave *bond_slave_has_mac(struct bonding *bond,
>> struct list_head *iter;
>> struct slave *tmp;
>>
>> - bond_for_each_slave(bond, tmp, iter)
>> + bond_for_each_slave_rcu(bond, tmp, iter)
>
> This suggests that rcu_read_lock() is always held here, however it's not
> the case, as far as I can tell:
>
> bond_release()/bond_uninit() ->
> __bond_release_one() ->
> bond_alb_deinit_slave() ->
> alb_change_hw_addr_on_detach() ->
> bond_slave_has_mac()
>
> There is nobody that takes rcu_read_lock() there. And, for example, when
> doing:
>
> echo -$slave > /sys/class/net/$bond_int/bonding/slaves
>
> nobody is holding it. And, btw, in bond_for_each_slave_rcu(), which is
> actually a wrapper for netdev_for_each_lower_private_rcu(), which calls
> netdev_lower_get_next_private_rcu(), which has:
>
> 4543 void *netdev_lower_get_next_private_rcu(struct net_device *dev,
> 4544 struct list_head **iter)
> 4545 {
> 4546 struct netdev_adjacent *lower;
> 4547 4548 WARN_ON_ONCE(!rcu_read_lock_held());
>
> so you should have seen that warning when trying to remove a slave in alb
> mode.
>
>> if (ether_addr_equal_64bits(mac, tmp->dev->dev_addr))
>> return tmp;
>>
>> --
>> 1.8.2.1
>>
>>
>>
oh, yes, has to do little more,thanks.
>
> .
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-09-30 11:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-29 11:45 [PATCH net-next 2/4] bonding: use RCU protection for alb xmit path Ding Tianhong
2013-09-30 10:52 ` Veaceslav Falico
2013-09-30 11: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).