Skip to content

Commit df01216

Browse files
davem330davem330
committed
Merge branch 'bonding_rcu'
Ding Tianhong says: ==================== bonding: rebuild the lock use for bond monitor Now the bond slave list is not protected by bond lock, only by RTNL, but the monitor still use the bond lock to protect the slave list, it is useless, according to the Veaceslav's opinion, there were three way to fix the protect problem: 1. add bond_master_upper_dev_link() and bond_upper_dev_unlink() in bond->lock, but it is unsafe to call call_netdevice_notifiers() in write lock. 2. remove unused bond->lock for monitor function, only use the exist rtnl lock(), it will take performance loss in fast path. 3. use RCU to protect the slave list, of course, performance is better, but in slow path, it is ignored. obviously the solution 1 is not fit here, I will consider the 2 and 3 solution. My principle is simple, if in fast path, RCU is better, otherwise in slow path, both is well, but according to the Jay Vosburgh's opinion, the monitor will loss performace if use RTNL to protect the all slave list, so remove the bond lock and replace with RCU. The second problem is the curr_slave_lock for bond, it is too old and unwanted in many place, because the curr_active_slave would only be changed in 3 place: 1. enslave slave. 2. release slave. 3. change active slave. all above were already holding bond lock, RTNL and curr_slave_lock together, it is tedious and no need to add so mach lock, when change the curr_active_slave, you have to hold the RTNL and curr_slave_lock together, and when you read the curr_active_slave, RTNL or curr_slave_lock, any one of them is no problem. for the stability, I did not change the logic for the monitor, all change is clear and simple, I have test the patch set for lockdep, it work well and stability. v2. accept the Jay Vosburgh's opinion, remove the RTNL and replace with RCU, also add some rcu function for bond use, so the patch set reach 10. v3. accept the Nikolay Aleksandrov's opinion, remove no needed bond_has_slave_rcu(), add protection for several 3ad mode handler functions and current_arp_slave. rebuild the bond_first_slave_rcu(), make it more clear. v4. because the struct netdev_adjacent should not be exist in netdevice.h, so I have to make a new function to support micro bond_first_slave_rcu(). also add a new patch to simplify the bond_resend_igmp_join_requests_delayed(). v5. according the Jay Vosburgh's opinion, in patch 2 and 6, the calling of notify peer is hardly to happen with the bond_xxx_commit() when the monitoring is running, so the performance impact about make two round trips to one trip on RTNL is minimal, no need to do that,the reason is very clear, so modify the patch 2 and 6, recover the notify peer in RTNL alone. ==================== Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> Signed-off-by: David S. Miller <davem@davemloft.net>
2 parents e57a784 + f236910 commit df01216

File tree

8 files changed

+130
-147
lines changed

8 files changed

+130
-147
lines changed

‎drivers/net/bonding/bond_3ad.c

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -147,11 +147,12 @@ static inline struct aggregator *__get_first_agg(struct port *port)
147147
struct bonding *bond = __get_bond_by_port(port);
148148
struct slave *first_slave;
149149

150-
// If there's no bond for this port, or bond has no slaves
150+
/* If there's no bond for this port, or bond has no slaves */
151151
if (bond == NULL)
152152
return NULL;
153-
first_slave = bond_first_slave(bond);
154-
153+
rcu_read_lock();
154+
first_slave = bond_first_slave_rcu(bond);
155+
rcu_read_unlock();
155156
return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
156157
}
157158

@@ -702,9 +703,13 @@ static struct aggregator *__get_active_agg(struct aggregator *aggregator)
702703
struct list_head *iter;
703704
struct slave *slave;
704705

705-
bond_for_each_slave(bond, slave, iter)
706-
if (SLAVE_AD_INFO(slave).aggregator.is_active)
706+
rcu_read_lock();
707+
bond_for_each_slave_rcu(bond, slave, iter)
708+
if (SLAVE_AD_INFO(slave).aggregator.is_active) {
709+
rcu_read_unlock();
707710
return &(SLAVE_AD_INFO(slave).aggregator);
711+
}
712+
rcu_read_unlock();
708713

709714
return NULL;
710715
}
@@ -1471,7 +1476,8 @@ static void ad_agg_selection_logic(struct aggregator *agg)
14711476
active = __get_active_agg(agg);
14721477
best = (active && agg_device_up(active)) ? active : NULL;
14731478

1474-
bond_for_each_slave(bond, slave, iter) {
1479+
rcu_read_lock();
1480+
bond_for_each_slave_rcu(bond, slave, iter) {
14751481
agg = &(SLAVE_AD_INFO(slave).aggregator);
14761482

14771483
agg->is_active = 0;
@@ -1505,7 +1511,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
15051511
active->is_active = 1;
15061512
}
15071513

1508-
// if there is new best aggregator, activate it
1514+
/* if there is new best aggregator, activate it */
15091515
if (best) {
15101516
pr_debug("best Agg=%d; P=%d; a k=%d; p k=%d; Ind=%d; Act=%d\n",
15111517
best->aggregator_identifier, best->num_of_ports,
@@ -1516,7 +1522,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
15161522
best->lag_ports, best->slave,
15171523
best->slave ? best->slave->dev->name : "NULL");
15181524

1519-
bond_for_each_slave(bond, slave, iter) {
1525+
bond_for_each_slave_rcu(bond, slave, iter) {
15201526
agg = &(SLAVE_AD_INFO(slave).aggregator);
15211527

15221528
pr_debug("Agg=%d; P=%d; a k=%d; p k=%d; Ind=%d; Act=%d\n",
@@ -1526,10 +1532,11 @@ static void ad_agg_selection_logic(struct aggregator *agg)
15261532
agg->is_individual, agg->is_active);
15271533
}
15281534

1529-
// check if any partner replys
1535+
/* check if any partner replys */
15301536
if (best->is_individual) {
15311537
pr_warning("%s: Warning: No 802.3ad response from the link partner for any adapters in the bond\n",
1532-
best->slave ? best->slave->bond->dev->name : "NULL");
1538+
best->slave ?
1539+
best->slave->bond->dev->name : "NULL");
15331540
}
15341541

15351542
best->is_active = 1;
@@ -1541,7 +1548,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
15411548
best->partner_oper_aggregator_key,
15421549
best->is_individual, best->is_active);
15431550

1544-
// disable the ports that were related to the former active_aggregator
1551+
/* disable the ports that were related to the former active_aggregator */
15451552
if (active) {
15461553
for (port = active->lag_ports; port;
15471554
port = port->next_port_in_aggregator) {
@@ -1565,6 +1572,8 @@ static void ad_agg_selection_logic(struct aggregator *agg)
15651572
}
15661573
}
15671574

1575+
rcu_read_unlock();
1576+
15681577
bond_3ad_set_carrier(bond);
15691578
}
15701579

@@ -2069,17 +2078,18 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
20692078
struct port *port;
20702079

20712080
read_lock(&bond->lock);
2081+
rcu_read_lock();
20722082

2073-
//check if there are any slaves
2083+
/* check if there are any slaves */
20742084
if (!bond_has_slaves(bond))
20752085
goto re_arm;
20762086

2077-
// check if agg_select_timer timer after initialize is timed out
2087+
/* check if agg_select_timer timer after initialize is timed out */
20782088
if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
2079-
slave = bond_first_slave(bond);
2089+
slave = bond_first_slave_rcu(bond);
20802090
port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
20812091

2082-
// select the active aggregator for the bond
2092+
/* select the active aggregator for the bond */
20832093
if (port) {
20842094
if (!port->slave) {
20852095
pr_warning("%s: Warning: bond's first port is uninitialized\n",
@@ -2093,8 +2103,8 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
20932103
bond_3ad_set_carrier(bond);
20942104
}
20952105

2096-
// for each port run the state machines
2097-
bond_for_each_slave(bond, slave, iter) {
2106+
/* for each port run the state machines */
2107+
bond_for_each_slave_rcu(bond, slave, iter) {
20982108
port = &(SLAVE_AD_INFO(slave).port);
20992109
if (!port->slave) {
21002110
pr_warning("%s: Warning: Found an uninitialized port\n",
@@ -2114,17 +2124,17 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
21142124
ad_mux_machine(port);
21152125
ad_tx_machine(port);
21162126

2117-
// turn off the BEGIN bit, since we already handled it
2127+
/* turn off the BEGIN bit, since we already handled it */
21182128
if (port->sm_vars & AD_PORT_BEGIN)
21192129
port->sm_vars &= ~AD_PORT_BEGIN;
21202130

21212131
__release_state_machine_lock(port);
21222132
}
21232133

21242134
re_arm:
2125-
queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
2126-
2135+
rcu_read_unlock();
21272136
read_unlock(&bond->lock);
2137+
queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
21282138
}
21292139

21302140
/**
@@ -2303,7 +2313,9 @@ int bond_3ad_set_carrier(struct bonding *bond)
23032313
struct aggregator *active;
23042314
struct slave *first_slave;
23052315

2306-
first_slave = bond_first_slave(bond);
2316+
rcu_read_lock();
2317+
first_slave = bond_first_slave_rcu(bond);
2318+
rcu_read_unlock();
23072319
if (!first_slave)
23082320
return 0;
23092321
active = __get_active_agg(&(SLAVE_AD_INFO(first_slave).aggregator));

‎drivers/net/bonding/bond_alb.c

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -469,7 +469,7 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
469469

470470
/* slave being removed should not be active at this point
471471
*
472-
* Caller must hold bond lock for read
472+
* Caller must hold rtnl.
473473
*/
474474
static void rlb_clear_slave(struct bonding *bond, struct slave *slave)
475475
{
@@ -815,7 +815,7 @@ static void rlb_rebalance(struct bonding *bond)
815815
for (; hash_index != RLB_NULL_INDEX;
816816
hash_index = client_info->used_next) {
817817
client_info = &(bond_info->rx_hashtbl[hash_index]);
818-
assigned_slave = rlb_next_rx_slave(bond);
818+
assigned_slave = __rlb_next_rx_slave(bond);
819819
if (assigned_slave && (client_info->slave != assigned_slave)) {
820820
client_info->slave = assigned_slave;
821821
client_info->ntt = 1;
@@ -1494,14 +1494,14 @@ void bond_alb_monitor(struct work_struct *work)
14941494
struct list_head *iter;
14951495
struct slave *slave;
14961496

1497-
read_lock(&bond->lock);
1498-
14991497
if (!bond_has_slaves(bond)) {
15001498
bond_info->tx_rebalance_counter = 0;
15011499
bond_info->lp_counter = 0;
15021500
goto re_arm;
15031501
}
15041502

1503+
rcu_read_lock();
1504+
15051505
bond_info->tx_rebalance_counter++;
15061506
bond_info->lp_counter++;
15071507

@@ -1514,7 +1514,7 @@ void bond_alb_monitor(struct work_struct *work)
15141514
*/
15151515
read_lock(&bond->curr_slave_lock);
15161516

1517-
bond_for_each_slave(bond, slave, iter)
1517+
bond_for_each_slave_rcu(bond, slave, iter)
15181518
alb_send_learning_packets(slave, slave->dev->dev_addr);
15191519

15201520
read_unlock(&bond->curr_slave_lock);
@@ -1527,7 +1527,7 @@ void bond_alb_monitor(struct work_struct *work)
15271527

15281528
read_lock(&bond->curr_slave_lock);
15291529

1530-
bond_for_each_slave(bond, slave, iter) {
1530+
bond_for_each_slave_rcu(bond, slave, iter) {
15311531
tlb_clear_slave(bond, slave, 1);
15321532
if (slave == bond->curr_active_slave) {
15331533
SLAVE_TLB_INFO(slave).load =
@@ -1551,11 +1551,9 @@ void bond_alb_monitor(struct work_struct *work)
15511551
* dev_set_promiscuity requires rtnl and
15521552
* nothing else. Avoid race with bond_close.
15531553
*/
1554-
read_unlock(&bond->lock);
1555-
if (!rtnl_trylock()) {
1556-
read_lock(&bond->lock);
1554+
rcu_read_unlock();
1555+
if (!rtnl_trylock())
15571556
goto re_arm;
1558-
}
15591557

15601558
bond_info->rlb_promisc_timeout_counter = 0;
15611559

@@ -1567,7 +1565,7 @@ void bond_alb_monitor(struct work_struct *work)
15671565
bond_info->primary_is_promisc = 0;
15681566

15691567
rtnl_unlock();
1570-
read_lock(&bond->lock);
1568+
rcu_read_lock();
15711569
}
15721570

15731571
if (bond_info->rlb_rebalance) {
@@ -1589,11 +1587,9 @@ void bond_alb_monitor(struct work_struct *work)
15891587
}
15901588
}
15911589
}
1592-
1590+
rcu_read_unlock();
15931591
re_arm:
15941592
queue_delayed_work(bond->wq, &bond->alb_work, alb_delta_in_ticks);
1595-
1596-
read_unlock(&bond->lock);
15971593
}
15981594

15991595
/* assumption: called before the slave is attached to the bond
@@ -1679,14 +1675,11 @@ void bond_alb_handle_link_change(struct bonding *bond, struct slave *slave, char
16791675
* If new_slave is NULL, caller must hold curr_slave_lock or
16801676
* bond->lock for write.
16811677
*
1682-
* If new_slave is not NULL, caller must hold RTNL, bond->lock for
1683-
* read and curr_slave_lock for write. Processing here may sleep, so
1684-
* no other locks may be held.
1678+
* If new_slave is not NULL, caller must hold RTNL, curr_slave_lock
1679+
* for write. Processing here may sleep, so no other locks may be held.
16851680
*/
16861681
void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave)
16871682
__releases(&bond->curr_slave_lock)
1688-
__releases(&bond->lock)
1689-
__acquires(&bond->lock)
16901683
__acquires(&bond->curr_slave_lock)
16911684
{
16921685
struct slave *swap_slave;
@@ -1722,7 +1715,6 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
17221715
tlb_clear_slave(bond, new_slave, 1);
17231716

17241717
write_unlock_bh(&bond->curr_slave_lock);
1725-
read_unlock(&bond->lock);
17261718

17271719
ASSERT_RTNL();
17281720

@@ -1748,11 +1740,9 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
17481740
/* swap mac address */
17491741
alb_swap_mac_addr(swap_slave, new_slave);
17501742
alb_fasten_mac_swap(bond, swap_slave, new_slave);
1751-
read_lock(&bond->lock);
17521743
} else {
17531744
/* set the new_slave to the bond mac address */
17541745
alb_set_slave_mac_addr(new_slave, bond->dev->dev_addr);
1755-
read_lock(&bond->lock);
17561746
alb_send_learning_packets(new_slave, bond->dev->dev_addr);
17571747
}
17581748

0 commit comments

Comments
 (0)