* [PATCH v3 net-next 1/2] bonding: RCUify bond_ab_arp_probe
2014-01-27 13:37 [PATCH v3 net-next 0/2] bonding: fix locking in bond_ab_arp_prob Veaceslav Falico
@ 2014-01-27 13:37 ` Veaceslav Falico
2014-01-27 13:37 ` [PATCH v3 net-next 2/2] bonding: restructure locking of bond_ab_arp_probe() Veaceslav Falico
2014-01-27 21:13 ` [PATCH v3 net-next 0/2] bonding: fix locking in bond_ab_arp_prob David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Veaceslav Falico @ 2014-01-27 13:37 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek
Currently bond_ab_arp_probe() is always called under rcu_read_lock(),
however to work with curr_active_slave we're still holding the
curr_slave_lock.
To remove that curr_slave_lock - rcu_dereference the bond's
curr_active_slave and use it further - so that we're sure the slave won't
go away, and we don't care if it will change in the meanwhile.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_main.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a7db819..27e6fdd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2605,25 +2605,21 @@ do_failover:
static void bond_ab_arp_probe(struct bonding *bond)
{
struct slave *slave, *before = NULL, *new_slave = NULL,
- *curr_arp_slave = rcu_dereference(bond->current_arp_slave);
+ *curr_arp_slave = rcu_dereference(bond->current_arp_slave),
+ *curr_active_slave = rcu_dereference(bond->curr_active_slave);
struct list_head *iter;
bool found = false;
- read_lock(&bond->curr_slave_lock);
-
- if (curr_arp_slave && bond->curr_active_slave)
+ if (curr_arp_slave && curr_active_slave)
pr_info("PROBE: c_arp %s && cas %s BAD\n",
curr_arp_slave->dev->name,
- bond->curr_active_slave->dev->name);
+ curr_active_slave->dev->name);
- if (bond->curr_active_slave) {
- bond_arp_send_all(bond, bond->curr_active_slave);
- read_unlock(&bond->curr_slave_lock);
+ if (curr_active_slave) {
+ bond_arp_send_all(bond, curr_active_slave);
return;
}
- read_unlock(&bond->curr_slave_lock);
-
/* if we don't have a curr_active_slave, search for the next available
* backup slave from the current_arp_slave and make it the candidate
* for becoming the curr_active_slave
--
1.8.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v3 net-next 2/2] bonding: restructure locking of bond_ab_arp_probe()
2014-01-27 13:37 [PATCH v3 net-next 0/2] bonding: fix locking in bond_ab_arp_prob Veaceslav Falico
2014-01-27 13:37 ` [PATCH v3 net-next 1/2] bonding: RCUify bond_ab_arp_probe Veaceslav Falico
@ 2014-01-27 13:37 ` Veaceslav Falico
2014-01-27 21:13 ` [PATCH v3 net-next 0/2] bonding: fix locking in bond_ab_arp_prob David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Veaceslav Falico @ 2014-01-27 13:37 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek
Currently we're calling it from under RCU context, however we're using some
functions that require rtnl to be held.
Fix this by restructuring the locking - don't call it under any locks,
aquire rcu_read_lock() if we're sending _only_ (i.e. we have the active
slave present), and use rtnl locking otherwise - if we need to modify
(in)active flags of a slave.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
Notes:
v2->v3:
Use rtnl_trylock(), not to race with queue cancelling.
v1->v2:
Add two steps - one for sending/rcu, another for modifying/rtnl.
drivers/net/bonding/bond_main.c | 57 ++++++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 21 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 27e6fdd..dd75615 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2599,17 +2599,18 @@ do_failover:
/*
* Send ARP probes for active-backup mode ARP monitor.
- *
- * Called with rcu_read_lock hold.
*/
-static void bond_ab_arp_probe(struct bonding *bond)
+static bool bond_ab_arp_probe(struct bonding *bond)
{
struct slave *slave, *before = NULL, *new_slave = NULL,
- *curr_arp_slave = rcu_dereference(bond->current_arp_slave),
- *curr_active_slave = rcu_dereference(bond->curr_active_slave);
+ *curr_arp_slave, *curr_active_slave;
struct list_head *iter;
bool found = false;
+ rcu_read_lock();
+ curr_arp_slave = rcu_dereference(bond->current_arp_slave);
+ curr_active_slave = rcu_dereference(bond->curr_active_slave);
+
if (curr_arp_slave && curr_active_slave)
pr_info("PROBE: c_arp %s && cas %s BAD\n",
curr_arp_slave->dev->name,
@@ -2617,23 +2618,32 @@ static void bond_ab_arp_probe(struct bonding *bond)
if (curr_active_slave) {
bond_arp_send_all(bond, curr_active_slave);
- return;
+ rcu_read_unlock();
+ return true;
}
+ rcu_read_unlock();
/* if we don't have a curr_active_slave, search for the next available
* backup slave from the current_arp_slave and make it the candidate
* for becoming the curr_active_slave
*/
+ if (!rtnl_trylock())
+ return false;
+ /* curr_arp_slave might have gone away */
+ curr_arp_slave = ACCESS_ONCE(bond->current_arp_slave);
+
if (!curr_arp_slave) {
- curr_arp_slave = bond_first_slave_rcu(bond);
- if (!curr_arp_slave)
- return;
+ curr_arp_slave = bond_first_slave(bond);
+ if (!curr_arp_slave) {
+ rtnl_unlock();
+ return true;
+ }
}
bond_set_slave_inactive_flags(curr_arp_slave);
- bond_for_each_slave_rcu(bond, slave, iter) {
+ bond_for_each_slave(bond, slave, iter) {
if (!found && !before && IS_UP(slave->dev))
before = slave;
@@ -2663,21 +2673,26 @@ static void bond_ab_arp_probe(struct bonding *bond)
if (!new_slave && before)
new_slave = before;
- if (!new_slave)
- return;
+ if (!new_slave) {
+ rtnl_unlock();
+ return true;
+ }
new_slave->link = BOND_LINK_BACK;
bond_set_slave_active_flags(new_slave);
bond_arp_send_all(bond, new_slave);
new_slave->jiffies = jiffies;
rcu_assign_pointer(bond->current_arp_slave, new_slave);
+ rtnl_unlock();
+
+ return true;
}
static void bond_activebackup_arp_mon(struct work_struct *work)
{
struct bonding *bond = container_of(work, struct bonding,
arp_work.work);
- bool should_notify_peers = false;
+ bool should_notify_peers = false, should_commit = false;
int delta_in_ticks;
delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
@@ -2686,12 +2701,11 @@ static void bond_activebackup_arp_mon(struct work_struct *work)
goto re_arm;
rcu_read_lock();
-
should_notify_peers = bond_should_notify_peers(bond);
+ should_commit = bond_ab_arp_inspect(bond);
+ rcu_read_unlock();
- if (bond_ab_arp_inspect(bond)) {
- rcu_read_unlock();
-
+ if (should_commit) {
/* Race avoidance with bond_close flush of workqueue */
if (!rtnl_trylock()) {
delta_in_ticks = 1;
@@ -2700,13 +2714,14 @@ static void bond_activebackup_arp_mon(struct work_struct *work)
}
bond_ab_arp_commit(bond);
-
rtnl_unlock();
- rcu_read_lock();
}
- bond_ab_arp_probe(bond);
- rcu_read_unlock();
+ if (!bond_ab_arp_probe(bond)) {
+ /* rtnl locking failed, re-arm */
+ delta_in_ticks = 1;
+ should_notify_peers = false;
+ }
re_arm:
if (bond->params.arp_interval)
--
1.8.4
^ permalink raw reply related [flat|nested] 4+ messages in thread