[PATCH net-next 0/3] bonding: rcu cleanups

[PATCH net-next 0/3] bonding: rcu cleanups

[Eric Dumazet]

RCU was added to bonding in linux-3.12 but lacked proper sparse annotations.

Using __rcu annotation actually helps to spot all accesses to bond->curr_active_slave
are correctly protected, with full sparse & LOCKDEP support.

Lets clean the code.

Eric Dumazet (3):
  bonding: get rid of bond_option_active_slave_get()
  bonding: use rcu_access_pointer() in bonding_show_mii_status()
  bonding: add proper __rcu annotation for curr_active_slave

 drivers/net/bonding/bond_alb.c     | 47 +++++++++++++++++++--------------
 drivers/net/bonding/bond_main.c    | 53 +++++++++++++++++++++-----------------
 drivers/net/bonding/bond_netlink.c | 21 +++++++++++----
 drivers/net/bonding/bond_options.c |  7 +----
 drivers/net/bonding/bond_sysfs.c   |  3 ++-
 drivers/net/bonding/bonding.h      |  7 +++--
 6 files changed, 81 insertions(+), 57 deletions(-)

-- 
2.0.0.526.g5318336

[PATCH net-next 1/3] bonding: get rid of bond_option_active_slave_get()

[Eric Dumazet]

Only keep bond_option_active_slave_get_rcu() helper.

bond_fill_info() uses a new bond_option_active_slave_get_ifindex()
helper.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/bonding/bond_netlink.c | 21 ++++++++++++++++-----
 drivers/net/bonding/bond_options.c |  5 -----
 drivers/net/bonding/bonding.h      |  1 -
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 5ab3c1847e67..4d97e23eb497 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -398,20 +398,31 @@ static size_t bond_get_size(const struct net_device *bond_dev)
 		0;
 }
 
+static int bond_option_active_slave_get_ifindex(struct bonding *bond)
+{
+	const struct net_device *slave;
+	int ifindex;
+
+	rcu_read_lock();
+	slave = bond_option_active_slave_get_rcu(bond);
+	ifindex = slave ? slave->ifindex : 0;
+	rcu_read_unlock();
+	return ifindex;
+}
+
 static int bond_fill_info(struct sk_buff *skb,
 			  const struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
-	struct net_device *slave_dev = bond_option_active_slave_get(bond);
-	struct nlattr *targets;
 	unsigned int packets_per_slave;
-	int i, targets_added;
+	int ifindex, i, targets_added;
+	struct nlattr *targets;
 
 	if (nla_put_u8(skb, IFLA_BOND_MODE, BOND_MODE(bond)))
 		goto nla_put_failure;
 
-	if (slave_dev &&
-	    nla_put_u32(skb, IFLA_BOND_ACTIVE_SLAVE, slave_dev->ifindex))
+	ifindex = bond_option_active_slave_get_ifindex(bond);
+	if (ifindex && nla_put_u32(skb, IFLA_BOND_ACTIVE_SLAVE, ifindex))
 		goto nla_put_failure;
 
 	if (nla_put_u32(skb, IFLA_BOND_MIIMON, bond->params.miimon))
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 540e0167bf24..b26271fd7b09 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -704,11 +704,6 @@ struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond)
 	return __bond_option_active_slave_get(bond, slave);
 }
 
-struct net_device *bond_option_active_slave_get(struct bonding *bond)
-{
-	return __bond_option_active_slave_get(bond, bond->curr_active_slave);
-}
-
 static int bond_option_active_slave_set(struct bonding *bond,
 					const struct bond_opt_value *newval)
 {
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 0b4d9cde0b05..713e2a99c661 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -514,7 +514,6 @@ unsigned int bond_get_num_tx_queues(void);
 int bond_netlink_init(void);
 void bond_netlink_fini(void);
 struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond);
-struct net_device *bond_option_active_slave_get(struct bonding *bond);
 const char *bond_slave_link_status(s8 link);
 bool bond_verify_device_path(struct net_device *start_dev,
 			     struct net_device *end_dev,
-- 
2.0.0.526.g5318336

Re: [PATCH net-next 1/3] bonding: get rid of bond_option_active_slave_get()

[Veaceslav Falico]

On Tue, Jul 15, 2014 at 02:46:42AM -0700, Eric Dumazet wrote:
>Only keep bond_option_active_slave_get_rcu() helper.
>
>bond_fill_info() uses a new bond_option_active_slave_get_ifindex()
>helper.
>
>Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Veaceslav Falico <vfalico@gmail.com>

>---
> drivers/net/bonding/bond_netlink.c | 21 ++++++++++++++++-----
> drivers/net/bonding/bond_options.c |  5 -----
> drivers/net/bonding/bonding.h      |  1 -
> 3 files changed, 16 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>index 5ab3c1847e67..4d97e23eb497 100644
>--- a/drivers/net/bonding/bond_netlink.c
>+++ b/drivers/net/bonding/bond_netlink.c
>@@ -398,20 +398,31 @@ static size_t bond_get_size(const struct net_device *bond_dev)
> 		0;
> }
>
>+static int bond_option_active_slave_get_ifindex(struct bonding *bond)
>+{
>+	const struct net_device *slave;
>+	int ifindex;
>+
>+	rcu_read_lock();
>+	slave = bond_option_active_slave_get_rcu(bond);
>+	ifindex = slave ? slave->ifindex : 0;
>+	rcu_read_unlock();
>+	return ifindex;
>+}
>+
> static int bond_fill_info(struct sk_buff *skb,
> 			  const struct net_device *bond_dev)
> {
> 	struct bonding *bond = netdev_priv(bond_dev);
>-	struct net_device *slave_dev = bond_option_active_slave_get(bond);
>-	struct nlattr *targets;
> 	unsigned int packets_per_slave;
>-	int i, targets_added;
>+	int ifindex, i, targets_added;
>+	struct nlattr *targets;
>
> 	if (nla_put_u8(skb, IFLA_BOND_MODE, BOND_MODE(bond)))
> 		goto nla_put_failure;
>
>-	if (slave_dev &&
>-	    nla_put_u32(skb, IFLA_BOND_ACTIVE_SLAVE, slave_dev->ifindex))
>+	ifindex = bond_option_active_slave_get_ifindex(bond);
>+	if (ifindex && nla_put_u32(skb, IFLA_BOND_ACTIVE_SLAVE, ifindex))
> 		goto nla_put_failure;
>
> 	if (nla_put_u32(skb, IFLA_BOND_MIIMON, bond->params.miimon))
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 540e0167bf24..b26271fd7b09 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -704,11 +704,6 @@ struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond)
> 	return __bond_option_active_slave_get(bond, slave);
> }
>
>-struct net_device *bond_option_active_slave_get(struct bonding *bond)
>-{
>-	return __bond_option_active_slave_get(bond, bond->curr_active_slave);
>-}
>-
> static int bond_option_active_slave_set(struct bonding *bond,
> 					const struct bond_opt_value *newval)
> {
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index 0b4d9cde0b05..713e2a99c661 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -514,7 +514,6 @@ unsigned int bond_get_num_tx_queues(void);
> int bond_netlink_init(void);
> void bond_netlink_fini(void);
> struct net_device *bond_option_active_slave_get_rcu(struct bonding *bond);
>-struct net_device *bond_option_active_slave_get(struct bonding *bond);
> const char *bond_slave_link_status(s8 link);
> bool bond_verify_device_path(struct net_device *start_dev,
> 			     struct net_device *end_dev,
>-- 
>2.0.0.526.g5318336
>

[PATCH net-next 2/3] bonding: use rcu_access_pointer() in bonding_show_mii_status()

[Eric Dumazet]

curr_active_slave is rcu protected, and bonding_show_mii_status() only
wants to check if pointer is NULL or not.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/bonding/bond_sysfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index daed52f68ce1..98db8edd9c75 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -492,8 +492,9 @@ static ssize_t bonding_show_mii_status(struct device *d,
 				       char *buf)
 {
 	struct bonding *bond = to_bond(d);
+	bool active = !!rcu_access_pointer(bond->curr_active_slave);
 
-	return sprintf(buf, "%s\n", bond->curr_active_slave ? "up" : "down");
+	return sprintf(buf, "%s\n", active ? "up" : "down");
 }
 static DEVICE_ATTR(mii_status, S_IRUGO, bonding_show_mii_status, NULL);
 
-- 
2.0.0.526.g5318336

Re: [PATCH net-next 2/3] bonding: use rcu_access_pointer() in bonding_show_mii_status()

[Veaceslav Falico]

On Tue, Jul 15, 2014 at 02:46:43AM -0700, Eric Dumazet wrote:
>curr_active_slave is rcu protected, and bonding_show_mii_status() only
>wants to check if pointer is NULL or not.
>
>Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Veaceslav Falico <vfalico@gmail.com>

>---
> drivers/net/bonding/bond_sysfs.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>index daed52f68ce1..98db8edd9c75 100644
>--- a/drivers/net/bonding/bond_sysfs.c
>+++ b/drivers/net/bonding/bond_sysfs.c
>@@ -492,8 +492,9 @@ static ssize_t bonding_show_mii_status(struct device *d,
> 				       char *buf)
> {
> 	struct bonding *bond = to_bond(d);
>+	bool active = !!rcu_access_pointer(bond->curr_active_slave);
>
>-	return sprintf(buf, "%s\n", bond->curr_active_slave ? "up" : "down");
>+	return sprintf(buf, "%s\n", active ? "up" : "down");
> }
> static DEVICE_ATTR(mii_status, S_IRUGO, bonding_show_mii_status, NULL);
>
>-- 
>2.0.0.526.g5318336
>

[PATCH net-next 3/3] bonding: add proper __rcu annotation for curr_active_slave

[Eric Dumazet]

RCU was added to bonding in linux-3.12 but lacked proper sparse annotations.

Using __rcu annotation actually helps to spot all accesses to bond->curr_active_slave
are correctly protected, with LOCKDEP support.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/bonding/bond_alb.c     | 47 +++++++++++++++++++--------------
 drivers/net/bonding/bond_main.c    | 53 +++++++++++++++++++++-----------------
 drivers/net/bonding/bond_options.c |  2 +-
 drivers/net/bonding/bonding.h      |  6 ++++-
 4 files changed, 63 insertions(+), 45 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 76c0dade233f..de5bd03925b4 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -448,11 +448,13 @@ static struct slave *__rlb_next_rx_slave(struct bonding *bond)
  */
 static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
 {
-	if (!bond->curr_active_slave)
+	struct slave *curr_active = bond_deref_active_protected(bond);
+
+	if (!curr_active)
 		return;
 
 	if (!bond->alb_info.primary_is_promisc) {
-		if (!dev_set_promiscuity(bond->curr_active_slave->dev, 1))
+		if (!dev_set_promiscuity(curr_active->dev, 1))
 			bond->alb_info.primary_is_promisc = 1;
 		else
 			bond->alb_info.primary_is_promisc = 0;
@@ -460,7 +462,7 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
 
 	bond->alb_info.rlb_promisc_timeout_counter = 0;
 
-	alb_send_learning_packets(bond->curr_active_slave, addr, true);
+	alb_send_learning_packets(curr_active, addr, true);
 }
 
 /* slave being removed should not be active at this point
@@ -509,7 +511,7 @@ static void rlb_clear_slave(struct bonding *bond, struct slave *slave)
 
 	write_lock_bh(&bond->curr_slave_lock);
 
-	if (slave != bond->curr_active_slave)
+	if (slave != bond_deref_active_protected(bond))
 		rlb_teach_disabled_mac_on_primary(bond, slave->dev->dev_addr);
 
 	write_unlock_bh(&bond->curr_slave_lock);
@@ -684,7 +686,7 @@ static struct slave *rlb_choose_channel(struct sk_buff *skb, struct bonding *bon
 			 * move the old client to primary (curr_active_slave) so
 			 * that the new client can be assigned to this entry.
 			 */
-			if (bond->curr_active_slave &&
+			if (curr_active_slave &&
 			    client_info->slave != curr_active_slave) {
 				client_info->slave = curr_active_slave;
 				rlb_update_client(client_info);
@@ -1221,7 +1223,7 @@ static void alb_change_hw_addr_on_detach(struct bonding *bond, struct slave *sla
  */
 static int alb_handle_addr_collision_on_attach(struct bonding *bond, struct slave *slave)
 {
-	struct slave *has_bond_addr = bond->curr_active_slave;
+	struct slave *has_bond_addr = rcu_access_pointer(bond->curr_active_slave);
 	struct slave *tmp_slave1, *free_mac_slave = NULL;
 	struct list_head *iter;
 
@@ -1575,7 +1577,7 @@ void bond_alb_monitor(struct work_struct *work)
 			 * use mac of the slave device.
 			 * In RLB mode, we always use strict matches.
 			 */
-			strict_match = (slave != bond->curr_active_slave ||
+			strict_match = (slave != rcu_access_pointer(bond->curr_active_slave) ||
 					bond_info->rlb_enabled);
 			alb_send_learning_packets(slave, slave->dev->dev_addr,
 						  strict_match);
@@ -1593,7 +1595,7 @@ void bond_alb_monitor(struct work_struct *work)
 
 		bond_for_each_slave_rcu(bond, slave, iter) {
 			tlb_clear_slave(bond, slave, 1);
-			if (slave == bond->curr_active_slave) {
+			if (slave == rcu_access_pointer(bond->curr_active_slave)) {
 				SLAVE_TLB_INFO(slave).load =
 					bond_info->unbalanced_load /
 						BOND_TLB_REBALANCE_INTERVAL;
@@ -1625,7 +1627,8 @@ void bond_alb_monitor(struct work_struct *work)
 			 * because a slave was disabled then
 			 * it can now leave promiscuous mode.
 			 */
-			dev_set_promiscuity(bond->curr_active_slave->dev, -1);
+			dev_set_promiscuity(rtnl_dereference(bond->curr_active_slave)->dev,
+					    -1);
 			bond_info->primary_is_promisc = 0;
 
 			rtnl_unlock();
@@ -1742,17 +1745,21 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
 	__acquires(&bond->curr_slave_lock)
 {
 	struct slave *swap_slave;
+	struct slave *curr_active;
 
-	if (bond->curr_active_slave == new_slave)
+	curr_active = rcu_dereference_protected(bond->curr_active_slave,
+						!new_slave ||
+						lockdep_is_held(&bond->curr_slave_lock));
+	if (curr_active == new_slave)
 		return;
 
-	if (bond->curr_active_slave && bond->alb_info.primary_is_promisc) {
-		dev_set_promiscuity(bond->curr_active_slave->dev, -1);
+	if (curr_active && bond->alb_info.primary_is_promisc) {
+		dev_set_promiscuity(curr_active->dev, -1);
 		bond->alb_info.primary_is_promisc = 0;
 		bond->alb_info.rlb_promisc_timeout_counter = 0;
 	}
 
-	swap_slave = bond->curr_active_slave;
+	swap_slave = curr_active;
 	rcu_assign_pointer(bond->curr_active_slave, new_slave);
 
 	if (!new_slave || !bond_has_slaves(bond))
@@ -1818,6 +1825,7 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct sockaddr *sa = addr;
+	struct slave *curr_active;
 	struct slave *swap_slave;
 	int res;
 
@@ -1834,23 +1842,24 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 	 * Otherwise we'll need to pass the new address to it and handle
 	 * duplications.
 	 */
-	if (!bond->curr_active_slave)
+	curr_active = rtnl_dereference(bond->curr_active_slave);
+	if (!curr_active)
 		return 0;
 
 	swap_slave = bond_slave_has_mac(bond, bond_dev->dev_addr);
 
 	if (swap_slave) {
-		alb_swap_mac_addr(swap_slave, bond->curr_active_slave);
-		alb_fasten_mac_swap(bond, swap_slave, bond->curr_active_slave);
+		alb_swap_mac_addr(swap_slave, curr_active);
+		alb_fasten_mac_swap(bond, swap_slave, curr_active);
 	} else {
-		alb_set_slave_mac_addr(bond->curr_active_slave, bond_dev->dev_addr);
+		alb_set_slave_mac_addr(curr_active, bond_dev->dev_addr);
 
 		read_lock(&bond->lock);
-		alb_send_learning_packets(bond->curr_active_slave,
+		alb_send_learning_packets(curr_active,
 					  bond_dev->dev_addr, false);
 		if (bond->alb_info.rlb_enabled) {
 			/* inform clients mac address has changed */
-			rlb_req_update_slave_clients(bond, bond->curr_active_slave);
+			rlb_req_update_slave_clients(bond, curr_active);
 		}
 		read_unlock(&bond->lock);
 	}
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 09dc3ef771a7..6a331bbcf43a 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -498,11 +498,11 @@ static int bond_set_promiscuity(struct bonding *bond, int inc)
 	int err = 0;
 
 	if (bond_uses_primary(bond)) {
+		struct slave *curr_active = bond_deref_active_protected(bond);
+
 		/* write lock already acquired */
-		if (bond->curr_active_slave) {
-			err = dev_set_promiscuity(bond->curr_active_slave->dev,
-						  inc);
-		}
+		if (curr_active)
+			err = dev_set_promiscuity(curr_active->dev, inc);
 	} else {
 		struct slave *slave;
 
@@ -524,11 +524,11 @@ static int bond_set_allmulti(struct bonding *bond, int inc)
 	int err = 0;
 
 	if (bond_uses_primary(bond)) {
+		struct slave *curr_active = bond_deref_active_protected(bond);
+
 		/* write lock already acquired */
-		if (bond->curr_active_slave) {
-			err = dev_set_allmulti(bond->curr_active_slave->dev,
-					       inc);
-		}
+		if (curr_active)
+			err = dev_set_allmulti(curr_active->dev, inc);
 	} else {
 		struct slave *slave;
 
@@ -713,7 +713,7 @@ out:
 static bool bond_should_change_active(struct bonding *bond)
 {
 	struct slave *prim = bond->primary_slave;
-	struct slave *curr = bond->curr_active_slave;
+	struct slave *curr = bond_deref_active_protected(bond);
 
 	if (!prim || !curr || curr->link != BOND_LINK_UP)
 		return true;
@@ -792,7 +792,11 @@ static bool bond_should_notify_peers(struct bonding *bond)
  */
 void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
 {
-	struct slave *old_active = bond->curr_active_slave;
+	struct slave *old_active;
+
+	old_active = rcu_dereference_protected(bond->curr_active_slave,
+					       !new_active ||
+					       lockdep_is_held(&bond->curr_slave_lock));
 
 	if (old_active == new_active)
 		return;
@@ -900,7 +904,7 @@ void bond_select_active_slave(struct bonding *bond)
 	int rv;
 
 	best_slave = bond_find_best_slave(bond);
-	if (best_slave != bond->curr_active_slave) {
+	if (best_slave != bond_deref_active_protected(bond)) {
 		bond_change_active_slave(bond, best_slave);
 		rv = bond_set_carrier(bond);
 		if (!rv)
@@ -1531,7 +1535,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		 * anyway (it holds no special properties of the bond device),
 		 * so we can change it without calling change_active_interface()
 		 */
-		if (!bond->curr_active_slave && new_slave->link == BOND_LINK_UP)
+		if (!rcu_access_pointer(bond->curr_active_slave) &&
+		    new_slave->link == BOND_LINK_UP)
 			rcu_assign_pointer(bond->curr_active_slave, new_slave);
 
 		break;
@@ -1602,7 +1607,7 @@ err_detach:
 	vlan_vids_del_by_dev(slave_dev, bond_dev);
 	if (bond->primary_slave == new_slave)
 		bond->primary_slave = NULL;
-	if (bond->curr_active_slave == new_slave) {
+	if (rcu_access_pointer(bond->curr_active_slave) == new_slave) {
 		block_netpoll_tx();
 		write_lock_bh(&bond->curr_slave_lock);
 		bond_change_active_slave(bond, NULL);
@@ -1704,7 +1709,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 		bond_is_active_slave(slave) ? "active" : "backup",
 		slave_dev->name);
 
-	oldcurrent = bond->curr_active_slave;
+	oldcurrent = rcu_access_pointer(bond->curr_active_slave);
 
 	bond->current_arp_slave = NULL;
 
@@ -1878,7 +1883,7 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in
 
 /*-------------------------------- Monitoring -------------------------------*/
 
-
+/* called with rcu_read_lock() */
 static int bond_miimon_inspect(struct bonding *bond)
 {
 	int link_state, commit = 0;
@@ -1886,7 +1891,7 @@ static int bond_miimon_inspect(struct bonding *bond)
 	struct slave *slave;
 	bool ignore_updelay;
 
-	ignore_updelay = !bond->curr_active_slave ? true : false;
+	ignore_updelay = !rcu_dereference(bond->curr_active_slave);
 
 	bond_for_each_slave_rcu(bond, slave, iter) {
 		slave->new_link = BOND_LINK_NOCHANGE;
@@ -2046,7 +2051,7 @@ static void bond_miimon_commit(struct bonding *bond)
 				bond_alb_handle_link_change(bond, slave,
 							    BOND_LINK_DOWN);
 
-			if (slave == bond->curr_active_slave)
+			if (slave == rcu_access_pointer(bond->curr_active_slave))
 				goto do_failover;
 
 			continue;
@@ -2416,7 +2421,7 @@ static void bond_loadbalance_arp_mon(struct work_struct *work)
 
 	rcu_read_lock();
 
-	oldcurrent = ACCESS_ONCE(bond->curr_active_slave);
+	oldcurrent = rcu_dereference(bond->curr_active_slave);
 	/* see if any of the previous devices are up now (i.e. they have
 	 * xmt and rcv traffic). the curr_active_slave does not come into
 	 * the picture unless it is null. also, slave->last_link_up is not
@@ -2607,8 +2612,8 @@ static void bond_ab_arp_commit(struct bonding *bond)
 
 		case BOND_LINK_UP:
 			trans_start = dev_trans_start(slave->dev);
-			if (bond->curr_active_slave != slave ||
-			    (!bond->curr_active_slave &&
+			if (rtnl_dereference(bond->curr_active_slave) != slave ||
+			    (!rtnl_dereference(bond->curr_active_slave) &&
 			     bond_time_in_interval(bond, trans_start, 1))) {
 				slave->link = BOND_LINK_UP;
 				if (bond->current_arp_slave) {
@@ -2621,7 +2626,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
 				pr_info("%s: link status definitely up for interface %s\n",
 					bond->dev->name, slave->dev->name);
 
-				if (!bond->curr_active_slave ||
+				if (!rtnl_dereference(bond->curr_active_slave) ||
 				    (slave == bond->primary_slave))
 					goto do_failover;
 
@@ -2640,7 +2645,7 @@ static void bond_ab_arp_commit(struct bonding *bond)
 			pr_info("%s: link status definitely down for interface %s, disabling it\n",
 				bond->dev->name, slave->dev->name);
 
-			if (slave == bond->curr_active_slave) {
+			if (slave == rtnl_dereference(bond->curr_active_slave)) {
 				bond->current_arp_slave = NULL;
 				goto do_failover;
 			}
@@ -3097,8 +3102,8 @@ static int bond_open(struct net_device *bond_dev)
 	if (bond_has_slaves(bond)) {
 		read_lock(&bond->curr_slave_lock);
 		bond_for_each_slave(bond, slave, iter) {
-			if (bond_uses_primary(bond)
-				&& (slave != bond->curr_active_slave)) {
+			if (bond_uses_primary(bond) &&
+			    slave != rcu_access_pointer(bond->curr_active_slave)) {
 				bond_set_slave_inactive_flags(slave,
 							      BOND_SLAVE_NOTIFY_NOW);
 			} else {
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index b26271fd7b09..f908e65e86c1 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -743,7 +743,7 @@ static int bond_option_active_slave_set(struct bonding *bond,
 		RCU_INIT_POINTER(bond->curr_active_slave, NULL);
 		bond_select_active_slave(bond);
 	} else {
-		struct slave *old_active = bond->curr_active_slave;
+		struct slave *old_active = bond_deref_active_protected(bond);
 		struct slave *new_active = bond_slave_get_rtnl(slave_dev);
 
 		BUG_ON(!new_active);
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 713e2a99c661..d03d2ae4d3af 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -194,7 +194,7 @@ struct slave {
  */
 struct bonding {
 	struct   net_device *dev; /* first - useful for panic debug */
-	struct   slave *curr_active_slave;
+	struct   slave __rcu *curr_active_slave;
 	struct   slave *current_arp_slave;
 	struct   slave *primary_slave;
 	bool     force_primary;
@@ -232,6 +232,10 @@ struct bonding {
 #define bond_slave_get_rtnl(dev) \
 	((struct slave *) rtnl_dereference(dev->rx_handler_data))
 
+#define bond_deref_active_protected(bond)				   \
+	rcu_dereference_protected(bond->curr_active_slave,		   \
+				  lockdep_is_held(&bond->curr_slave_lock))
+
 struct bond_vlan_tag {
 	__be16		vlan_proto;
 	unsigned short	vlan_id;
-- 
2.0.0.526.g5318336

Re: [PATCH net-next 3/3] bonding: add proper __rcu annotation for curr_active_slave

[Veaceslav Falico]

On Tue, Jul 15, 2014 at 02:46:44AM -0700, Eric Dumazet wrote:
>RCU was added to bonding in linux-3.12 but lacked proper sparse annotations.
>
>Using __rcu annotation actually helps to spot all accesses to bond->curr_active_slave
>are correctly protected, with LOCKDEP support.
>
>Signed-off-by: Eric Dumazet <edumazet@google.com>

Thanks a lot for cleaning this up, it's really a huge mess here...

One question though:

...snip...
>@@ -2046,7 +2051,7 @@ static void bond_miimon_commit(struct bonding *bond)
> 				bond_alb_handle_link_change(bond, slave,
> 							    BOND_LINK_DOWN);
>
>-			if (slave == bond->curr_active_slave)
>+			if (slave == rcu_access_pointer(bond->curr_active_slave))

I guess you've meant rtnl_dereference()? As bond_miimon_commit() is ujnder
rtnl, not under rcu.

> 				goto do_failover;
>
> 			continue;

Re: [PATCH net-next 3/3] bonding: add proper __rcu annotation for curr_active_slave

[Eric Dumazet]

On Tue, 2014-07-15 at 12:38 +0200, Veaceslav Falico wrote:
> On Tue, Jul 15, 2014 at 02:46:44AM -0700, Eric Dumazet wrote:
> >RCU was added to bonding in linux-3.12 but lacked proper sparse annotations.
> >
> >Using __rcu annotation actually helps to spot all accesses to bond->curr_active_slave
> >are correctly protected, with LOCKDEP support.
> >
> >Signed-off-by: Eric Dumazet <edumazet@google.com>
> 
> Thanks a lot for cleaning this up, it's really a huge mess here...
> 
> One question though:
> 
> ...snip...
> >@@ -2046,7 +2051,7 @@ static void bond_miimon_commit(struct bonding *bond)
> > 				bond_alb_handle_link_change(bond, slave,
> > 							    BOND_LINK_DOWN);
> >
> >-			if (slave == bond->curr_active_slave)
> >+			if (slave == rcu_access_pointer(bond->curr_active_slave))
> 
> I guess you've meant rtnl_dereference()? As bond_miimon_commit() is ujnder
> rtnl, not under rcu.

This does not really matter here, as we only perform an equality test.

(We dont read the pointer, then dereference it)

rcu_access_pointer() is fine in this context, this makes sparse happy
mostly.

Re: [PATCH net-next 3/3] bonding: add proper __rcu annotation for curr_active_slave

[Veaceslav Falico]

On Tue, Jul 15, 2014 at 12:52:49PM +0200, Eric Dumazet wrote:
>On Tue, 2014-07-15 at 12:38 +0200, Veaceslav Falico wrote:
>> On Tue, Jul 15, 2014 at 02:46:44AM -0700, Eric Dumazet wrote:
>> >RCU was added to bonding in linux-3.12 but lacked proper sparse annotations.
>> >
>> >Using __rcu annotation actually helps to spot all accesses to bond->curr_active_slave
>> >are correctly protected, with LOCKDEP support.
>> >
>> >Signed-off-by: Eric Dumazet <edumazet@google.com>
>>
>> Thanks a lot for cleaning this up, it's really a huge mess here...
>>
>> One question though:
>>
>> ...snip...
>> >@@ -2046,7 +2051,7 @@ static void bond_miimon_commit(struct bonding *bond)
>> > 				bond_alb_handle_link_change(bond, slave,
>> > 							    BOND_LINK_DOWN);
>> >
>> >-			if (slave == bond->curr_active_slave)
>> >+			if (slave == rcu_access_pointer(bond->curr_active_slave))
>>
>> I guess you've meant rtnl_dereference()? As bond_miimon_commit() is ujnder
>> rtnl, not under rcu.
>
>This does not really matter here, as we only perform an equality test.
>
>(We dont read the pointer, then dereference it)
>
>rcu_access_pointer() is fine in this context, this makes sparse happy
>mostly.

Fair enough.

Acked-by: Veaceslav Falico <vfalico@gmail.com>

Re: [PATCH net-next 0/3] bonding: rcu cleanups

[Nikolay Aleksandrov]

On 07/15/2014 11:46 AM, Eric Dumazet wrote:
> RCU was added to bonding in linux-3.12 but lacked proper sparse annotations.
> 
> Using __rcu annotation actually helps to spot all accesses to bond->curr_active_slave
> are correctly protected, with full sparse & LOCKDEP support.
> 
> Lets clean the code.
> 
> Eric Dumazet (3):
>   bonding: get rid of bond_option_active_slave_get()
>   bonding: use rcu_access_pointer() in bonding_show_mii_status()
>   bonding: add proper __rcu annotation for curr_active_slave
> 
>  drivers/net/bonding/bond_alb.c     | 47 +++++++++++++++++++--------------
>  drivers/net/bonding/bond_main.c    | 53 +++++++++++++++++++++-----------------
>  drivers/net/bonding/bond_netlink.c | 21 +++++++++++----
>  drivers/net/bonding/bond_options.c |  7 +----
>  drivers/net/bonding/bond_sysfs.c   |  3 ++-
>  drivers/net/bonding/bonding.h      |  7 +++--
>  6 files changed, 81 insertions(+), 57 deletions(-)
> 
Nice clean up, I also gave it a quick run. I believe similar clean up would
be beneficial for current_arp_slave at some point.

Anyhow, thanks Eric!

For the set
Reviewed-by: Nikolay Aleksandrov <nikolay@redhat.com>

Re: [PATCH net-next 0/3] bonding: rcu cleanups

[Eric Dumazet]

On Tue, 2014-07-15 at 12:55 +0200, Nikolay Aleksandrov wrote:
> Nice clean up, I also gave it a quick run. I believe similar clean up would
> be beneficial for current_arp_slave at some point.

Yes this one should be much easier, I'll send it, thanks.