[PATCH net-next 0/3] bonding: Fix RTNL: assertion failed at net/core/rtnetlink.c

[PATCH net-next 0/3] bonding: Fix RTNL: assertion failed at net/core/rtnetlink.c

[Ding Tianhong]

The problem was introduced by the commit 1d3ee88ae0d
(bonding: add netlink attributes to slave link dev).
The bond_set_active_slave() and bond_set_backup_slave()
will use rtmsg_ifinfo to send slave's states, so these
two functions should be called in RTNL.

In 802.3ad mode, acquiring RTNL for the __enable_port and
__disable_port cases is difficult, as those calls generally
already hold the state machine lock, and cannot unconditionally
call rtnl_lock because either they already hold RTNL (for calls
via bond_3ad_unbind_slave) or due to the potential for deadlock
with bond_3ad_adapter_speed_changed, bond_3ad_adapter_duplex_changed,
bond_3ad_link_change, or bond_3ad_update_lacp_rate.  All four of
those are called with RTNL held, and acquire the state machine lock
second.  The calling contexts for __enable_port and __disable_port
already hold the state machine lock, and may or may not need RTNL.

According to the Jay's opinion, I don't think it is a problem that
the slave don't send notify message synchronously when the status
changed, normally the state machine is running every 100 ms, send
the notify message at the end of the state machine if the slave's
state changed should be better.

Ding Tianhong (3):
  bonding: add bond_set_slave_state/flags()
  bonding: add new slave param and  bond_slave_state_notify()
  bonding: Fix the RTNL assertion failed for 802.3ad state machine

 drivers/net/bonding/bond_3ad.c |  5 ++--
 drivers/net/bonding/bonding.h  | 68 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 3 deletions(-)

-- 
1.8.0

[PATCH net-next 1/3] bonding: add bond_set_slave_state/flags()

[Ding Tianhong]

The new function could change the slave state and flags, then call
rtmsg_ifinfo() according to the input parameters notify.

Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bonding.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 86ccfb9..d210124 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -303,6 +303,18 @@ static inline void bond_set_backup_slave(struct slave *slave)
 	}
 }
 
+static inline void bond_set_slave_state(struct slave *slave,
+					int slave_state, bool notify)
+{
+	if (slave->backup != slave_state)
+		slave->backup = slave_state;
+	else
+		return;
+
+	if (notify)
+		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
+}
+
 static inline void bond_slave_state_change(struct bonding *bond)
 {
 	struct list_head *iter;
@@ -408,6 +420,20 @@ static inline void bond_set_slave_active_flags(struct slave *slave)
 	slave->inactive = 0;
 }
 
+static inline void bond_set_slave_flags(struct slave *slave,
+					int state, bool notify)
+
+{
+	if (state == BOND_STATE_ACTIVE) {
+		bond_set_slave_state(slave, state, notify);
+		slave->inactive = 0;
+	} else if (state == BOND_STATE_BACKUP && !bond_is_lb(slave->bond)) {
+		bond_set_slave_state(slave, state, notify);
+		if (!slave->bond->params.all_slaves_active)
+			slave->inactive = 1;
+	}
+}
+
 static inline bool bond_is_slave_inactive(struct slave *slave)
 {
 	return slave->inactive;
-- 
1.8.0

Re: [PATCH net-next 1/3] bonding: add bond_set_slave_state/flags()

[Jay Vosburgh]

Ding Tianhong <dingtianhong@huawei.com> wrote:

>The new function could change the slave state and flags, then call
>rtmsg_ifinfo() according to the input parameters notify.
>
>Cc: Jay Vosburgh <fubar@us.ibm.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bonding.h | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index 86ccfb9..d210124 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -303,6 +303,18 @@ static inline void bond_set_backup_slave(struct slave *slave)
> 	}
> }
>
>+static inline void bond_set_slave_state(struct slave *slave,
>+					int slave_state, bool notify)
>+{
>+	if (slave->backup != slave_state)
>+		slave->backup = slave_state;
>+	else
>+		return;
>+
>+	if (notify)
>+		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);

	I think this would be clearer if coded as:

	if (slave->backup == slave_slave)
		return;

	slave->backup = slave_state;
	if (notify)
		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);

>+}
>+
> static inline void bond_slave_state_change(struct bonding *bond)
> {
> 	struct list_head *iter;
>@@ -408,6 +420,20 @@ static inline void bond_set_slave_active_flags(struct slave *slave)
> 	slave->inactive = 0;
> }
>
>+static inline void bond_set_slave_flags(struct slave *slave,
>+					int state, bool notify)
>+
>+{
>+	if (state == BOND_STATE_ACTIVE) {
>+		bond_set_slave_state(slave, state, notify);
>+		slave->inactive = 0;
>+	} else if (state == BOND_STATE_BACKUP && !bond_is_lb(slave->bond)) {
>+		bond_set_slave_state(slave, state, notify);
>+		if (!slave->bond->params.all_slaves_active)
>+			slave->inactive = 1;
>+	}
>+}

	As I said in my other reply, I don't see why this shouldn't be
integrated into the existing state change functions instead of creating
a new function.

	-J

> static inline bool bond_is_slave_inactive(struct slave *slave)
> {
> 	return slave->inactive;
>-- 
>1.8.0

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next 1/3] bonding: add bond_set_slave_state/flags()

[Ding Tianhong]

On 2014/2/18 10:08, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@huawei.com> wrote:
> 
>> The new function could change the slave state and flags, then call
>> rtmsg_ifinfo() according to the input parameters notify.
>>
>> Cc: Jay Vosburgh <fubar@us.ibm.com>
>> Cc: Veaceslav Falico <vfalico@redhat.com>
>> Cc: Andy Gospodarek <andy@greyhouse.net>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bonding.h | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>> index 86ccfb9..d210124 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -303,6 +303,18 @@ static inline void bond_set_backup_slave(struct slave *slave)
>> 	}
>> }
>>
>> +static inline void bond_set_slave_state(struct slave *slave,
>> +					int slave_state, bool notify)
>> +{
>> +	if (slave->backup != slave_state)
>> +		slave->backup = slave_state;
>> +	else
>> +		return;
>> +
>> +	if (notify)
>> +		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
> 
> 	I think this would be clearer if coded as:
> 
> 	if (slave->backup == slave_slave)
> 		return;
> 
> 	slave->backup = slave_state;
> 	if (notify)
> 		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
> 
Yes, more simple.

>> +}
>> +
>> static inline void bond_slave_state_change(struct bonding *bond)
>> {
>> 	struct list_head *iter;
>> @@ -408,6 +420,20 @@ static inline void bond_set_slave_active_flags(struct slave *slave)
>> 	slave->inactive = 0;
>> }
>>
>> +static inline void bond_set_slave_flags(struct slave *slave,
>> +					int state, bool notify)
>> +
>> +{
>> +	if (state == BOND_STATE_ACTIVE) {
>> +		bond_set_slave_state(slave, state, notify);
>> +		slave->inactive = 0;
>> +	} else if (state == BOND_STATE_BACKUP && !bond_is_lb(slave->bond)) {
>> +		bond_set_slave_state(slave, state, notify);
>> +		if (!slave->bond->params.all_slaves_active)
>> +			slave->inactive = 1;
>> +	}
>> +}
> 
> 	As I said in my other reply, I don't see why this shouldn't be
> integrated into the existing state change functions instead of creating
> a new function.
> 
> 	-J

Ok, thanks.

Ding

> 
>> static inline bool bond_is_slave_inactive(struct slave *slave)
>> {
>> 	return slave->inactive;
>> -- 
>> 1.8.0
> 
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

[PATCH net-next 2/3] bonding: add new slave param and bond_slave_state_notify()

[Ding Tianhong]

Add a new slave parameter which called should_notify, if the slave's state
changed and don't notify yet, the parameter will be set to 1, and then if
the slave's state changed again, the param will be set to 0, it indicate that
the slave's state has been restored, no need to notify any one.

The bond_slave_state_notify() will check whether the status changed and then
decide to notify or not.

Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bonding.h | 44 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index d210124..4d0cd41 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -195,7 +195,8 @@ struct slave {
 	s8     new_link;
 	u8     backup:1,   /* indicates backup slave. Value corresponds with
 			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
-	       inactive:1; /* indicates inactive slave */
+	       inactive:1, /* indicates inactive slave */
+	       should_notify:1; /* indicateds whether the state changed */
 	u8     duplex;
 	u32    original_mtu;
 	u32    link_failure_count;
@@ -311,8 +312,47 @@ static inline void bond_set_slave_state(struct slave *slave,
 	else
 		return;
 
-	if (notify)
+	if (notify) {
 		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
+		slave->should_notify = 0;
+	} else {
+		if (slave->should_notify)
+			slave->should_notify = 0;
+		else
+			slave->should_notify = 1;
+	}
+}
+
+static inline void bond_slave_state_notify(struct bonding *bond,
+					   bool rtnl_locked)
+{
+	struct list_head *iter;
+	struct slave *tmp;
+
+	rcu_read_lock();
+	bond_for_each_slave_rcu(bond, tmp, iter) {
+		if (tmp->should_notify) {
+			rcu_read_unlock();
+			goto should_notify;
+		}
+	}
+	rcu_read_unlock();
+	return;
+
+should_notify:
+
+	if (!rtnl_locked && !rtnl_trylock())
+		return;
+
+	bond_for_each_slave(bond, tmp, iter) {
+		if (tmp->should_notify) {
+			rtmsg_ifinfo(RTM_NEWLINK, tmp->dev, 0, GFP_KERNEL);
+			tmp->should_notify = 0;
+		}
+	}
+
+	if (!rtnl_locked)
+		rtnl_unlock();
 }
 
 static inline void bond_slave_state_change(struct bonding *bond)
-- 
1.8.0

Re: [PATCH net-next 2/3] bonding: add new slave param and bond_slave_state_notify()

[Jay Vosburgh]

Ding Tianhong <dingtianhong@huawei.com> wrote:

>Add a new slave parameter which called should_notify, if the slave's state
>changed and don't notify yet, the parameter will be set to 1, and then if
>the slave's state changed again, the param will be set to 0, it indicate that
>the slave's state has been restored, no need to notify any one.
>
>The bond_slave_state_notify() will check whether the status changed and then
>decide to notify or not.
>
>Cc: Jay Vosburgh <fubar@us.ibm.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bonding.h | 44 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 42 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>index d210124..4d0cd41 100644
>--- a/drivers/net/bonding/bonding.h
>+++ b/drivers/net/bonding/bonding.h
>@@ -195,7 +195,8 @@ struct slave {
> 	s8     new_link;
> 	u8     backup:1,   /* indicates backup slave. Value corresponds with
> 			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
>-	       inactive:1; /* indicates inactive slave */
>+	       inactive:1, /* indicates inactive slave */
>+	       should_notify:1; /* indicateds whether the state changed */
> 	u8     duplex;
> 	u32    original_mtu;
> 	u32    link_failure_count;
>@@ -311,8 +312,47 @@ static inline void bond_set_slave_state(struct slave *slave,
> 	else
> 		return;
>
>-	if (notify)
>+	if (notify) {
> 		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
>+		slave->should_notify = 0;
>+	} else {
>+		if (slave->should_notify)
>+			slave->should_notify = 0;
>+		else
>+			slave->should_notify = 1;
>+	}
>+}
>+
>+static inline void bond_slave_state_notify(struct bonding *bond,
>+					   bool rtnl_locked)
>+{
>+	struct list_head *iter;
>+	struct slave *tmp;
>+
>+	rcu_read_lock();
>+	bond_for_each_slave_rcu(bond, tmp, iter) {
>+		if (tmp->should_notify) {
>+			rcu_read_unlock();
>+			goto should_notify;
>+		}
>+	}
>+	rcu_read_unlock();
>+	return;
>+
>+should_notify:
>+
>+	if (!rtnl_locked && !rtnl_trylock())
>+		return;
>+
>+	bond_for_each_slave(bond, tmp, iter) {
>+		if (tmp->should_notify) {
>+			rtmsg_ifinfo(RTM_NEWLINK, tmp->dev, 0, GFP_KERNEL);
>+			tmp->should_notify = 0;
>+		}
>+	}
>+
>+	if (!rtnl_locked)
>+		rtnl_unlock();
> }

	This function (bond_slave_state_notify) seems overly complicated
given that there appears to be only one caller.  In particular, why
bother with the "rtnl_locked" flag at all, when it is never called with
it set to true?  Really, with only one caller (in patch 3 of the
series), I'm not convinced this even needs to be a separate function.

	-J

>
> static inline void bond_slave_state_change(struct bonding *bond)
>-- 
>1.8.0

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next 2/3] bonding: add new slave param and bond_slave_state_notify()

[Ding Tianhong]

On 2014/2/18 10:07, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@huawei.com> wrote:
> 
>> Add a new slave parameter which called should_notify, if the slave's state
>> changed and don't notify yet, the parameter will be set to 1, and then if
>> the slave's state changed again, the param will be set to 0, it indicate that
>> the slave's state has been restored, no need to notify any one.
>>
>> The bond_slave_state_notify() will check whether the status changed and then
>> decide to notify or not.
>>
>> Cc: Jay Vosburgh <fubar@us.ibm.com>
>> Cc: Veaceslav Falico <vfalico@redhat.com>
>> Cc: Andy Gospodarek <andy@greyhouse.net>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bonding.h | 44 +++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>> index d210124..4d0cd41 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -195,7 +195,8 @@ struct slave {
>> 	s8     new_link;
>> 	u8     backup:1,   /* indicates backup slave. Value corresponds with
>> 			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
>> -	       inactive:1; /* indicates inactive slave */
>> +	       inactive:1, /* indicates inactive slave */
>> +	       should_notify:1; /* indicateds whether the state changed */
>> 	u8     duplex;
>> 	u32    original_mtu;
>> 	u32    link_failure_count;
>> @@ -311,8 +312,47 @@ static inline void bond_set_slave_state(struct slave *slave,
>> 	else
>> 		return;
>>
>> -	if (notify)
>> +	if (notify) {
>> 		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
>> +		slave->should_notify = 0;
>> +	} else {
>> +		if (slave->should_notify)
>> +			slave->should_notify = 0;
>> +		else
>> +			slave->should_notify = 1;
>> +	}
>> +}
>> +
>> +static inline void bond_slave_state_notify(struct bonding *bond,
>> +					   bool rtnl_locked)
>> +{
>> +	struct list_head *iter;
>> +	struct slave *tmp;
>> +
>> +	rcu_read_lock();
>> +	bond_for_each_slave_rcu(bond, tmp, iter) {
>> +		if (tmp->should_notify) {
>> +			rcu_read_unlock();
>> +			goto should_notify;
>> +		}
>> +	}
>> +	rcu_read_unlock();
>> +	return;
>> +
>> +should_notify:
>> +
>> +	if (!rtnl_locked && !rtnl_trylock())
>> +		return;
>> +
>> +	bond_for_each_slave(bond, tmp, iter) {
>> +		if (tmp->should_notify) {
>> +			rtmsg_ifinfo(RTM_NEWLINK, tmp->dev, 0, GFP_KERNEL);
>> +			tmp->should_notify = 0;
>> +		}
>> +	}
>> +
>> +	if (!rtnl_locked)
>> +		rtnl_unlock();
>> }
> 
> 	This function (bond_slave_state_notify) seems overly complicated
> given that there appears to be only one caller.  In particular, why
> bother with the "rtnl_locked" flag at all, when it is never called with
> it set to true?  Really, with only one caller (in patch 3 of the
> series), I'm not convinced this even needs to be a separate function.
> 
> 	-J
> 
In my original opinion, I think it may be used in RTNL for other monitor,
so add this one, I will remove it, thanks.

Regards
Ding

>>
>> static inline void bond_slave_state_change(struct bonding *bond)
>> -- 
>> 1.8.0
> 
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
> 
> 
> .
> 

[PATCH net-next 3/3] bonding: Fix the RTNL assertion failed for 802.3ad state machine

[Ding Tianhong]

The 802.3ad state machine don't run in RTNL, but when the slave's
state changed, the rtmsg_ifinfo will be called, it will cause
warning message because the RTML is not locked, acquiring RTNL
for the __enable_port and __disable_port cases is difficult, as
those calls generally already hold the state machine lock, and
can't unconditionally call rtnl_lock because either they already
hold RTNL (for calls via bond_3ad_unbind_slave) or due to the
potential for deadlock with bond_3ad_adapter_speed_changed,
bond_3ad_adapter_duplex_changed, bond_3ad_link_change, or
bond_3ad_update_lacp_rate. All four of those are called with RTNL
held, and acquire the state machine lock second, The calling contexts for
__enable_port and __disable_port already hold the state machine lock,
and may or may not need RTNL.

So according to the Jay's opinion, the __enable_port and __disable_port
should not call rtmsg_ifinfo in the state machine lock, any change in
the state of slave could set a flag in the slave, it will indicated that
an rtmsg_ifinfo should be called at the end of the state machine.

Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_3ad.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index cce1f1b..e80b78f 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -181,7 +181,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
  */
 static inline void __disable_port(struct port *port)
 {
-	bond_set_slave_inactive_flags(port->slave);
+	bond_set_slave_flags(port->slave, BOND_STATE_BACKUP, false);
 }
 
 /**
@@ -193,7 +193,7 @@ static inline void __enable_port(struct port *port)
 	struct slave *slave = port->slave;
 
 	if ((slave->link == BOND_LINK_UP) && IS_UP(slave->dev))
-		bond_set_slave_active_flags(slave);
+		bond_set_slave_flags(slave, BOND_STATE_ACTIVE, false);
 }
 
 /**
@@ -2123,6 +2123,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 re_arm:
 	rcu_read_unlock();
 	read_unlock(&bond->lock);
+	bond_slave_state_notify(bond, false);
 	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
 }
 
-- 
1.8.0

Re: [PATCH net-next 3/3] bonding: Fix the RTNL assertion failed for 802.3ad state machine

[Jay Vosburgh]

Ding Tianhong <dingtianhong@huawei.com> wrote:

>The 802.3ad state machine don't run in RTNL, but when the slave's
>state changed, the rtmsg_ifinfo will be called, it will cause
>warning message because the RTML is not locked, acquiring RTNL
>for the __enable_port and __disable_port cases is difficult, as
>those calls generally already hold the state machine lock, and
>can't unconditionally call rtnl_lock because either they already
>hold RTNL (for calls via bond_3ad_unbind_slave) or due to the
>potential for deadlock with bond_3ad_adapter_speed_changed,
>bond_3ad_adapter_duplex_changed, bond_3ad_link_change, or
>bond_3ad_update_lacp_rate. All four of those are called with RTNL
>held, and acquire the state machine lock second, The calling contexts for
>__enable_port and __disable_port already hold the state machine lock,
>and may or may not need RTNL.
>
>So according to the Jay's opinion, the __enable_port and __disable_port
>should not call rtmsg_ifinfo in the state machine lock, any change in
>the state of slave could set a flag in the slave, it will indicated that
>an rtmsg_ifinfo should be called at the end of the state machine.

	To clarify, my opinion being referenced here was really asking
Scott Feldman <sfeldma@cumulusnetworks.com> if: (a) the calls had to be
synchronous, and, (b) if the intermediate calls to adjust flags within
the ARP monitor "cycle through slaves looking for a functional slave"
all required notifications.  My suspicion is that the answer to both of
those is "no," but I haven't heard from Scott.

>Cc: Jay Vosburgh <fubar@us.ibm.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bond_3ad.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index cce1f1b..e80b78f 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -181,7 +181,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
>  */
> static inline void __disable_port(struct port *port)
> {
>-	bond_set_slave_inactive_flags(port->slave);
>+	bond_set_slave_flags(port->slave, BOND_STATE_BACKUP, false);
> }
>
> /**
>@@ -193,7 +193,7 @@ static inline void __enable_port(struct port *port)
> 	struct slave *slave = port->slave;
>
> 	if ((slave->link == BOND_LINK_UP) && IS_UP(slave->dev))
>-		bond_set_slave_active_flags(slave);
>+		bond_set_slave_flags(slave, BOND_STATE_ACTIVE, false);

	I don't agree that we need to have two separate systems (your
new bond_set_slave_flags plus bond_set_slave_{active,inactive}_flags)
that both tweak the "active" or "inactive" flags for a slave.  It would
be much cleaner and consistent with the current code to add a "notify"
boolean to the existing functions.

	-J

> }
>
> /**
>@@ -2123,6 +2123,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
> re_arm:
> 	rcu_read_unlock();
> 	read_unlock(&bond->lock);
>+	bond_slave_state_notify(bond, false);
> 	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
> }
>
>-- 
>1.8.0

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next 3/3] bonding: Fix the RTNL assertion failed for 802.3ad state machine

[Ding Tianhong]

On 2014/2/18 10:06, Jay Vosburgh wrote:
> Ding Tianhong <dingtianhong@huawei.com> wrote:
> 
>> The 802.3ad state machine don't run in RTNL, but when the slave's
>> state changed, the rtmsg_ifinfo will be called, it will cause
>> warning message because the RTML is not locked, acquiring RTNL
>> for the __enable_port and __disable_port cases is difficult, as
>> those calls generally already hold the state machine lock, and
>> can't unconditionally call rtnl_lock because either they already
>> hold RTNL (for calls via bond_3ad_unbind_slave) or due to the
>> potential for deadlock with bond_3ad_adapter_speed_changed,
>> bond_3ad_adapter_duplex_changed, bond_3ad_link_change, or
>> bond_3ad_update_lacp_rate. All four of those are called with RTNL
>> held, and acquire the state machine lock second, The calling contexts for
>> __enable_port and __disable_port already hold the state machine lock,
>> and may or may not need RTNL.
>>
>> So according to the Jay's opinion, the __enable_port and __disable_port
>> should not call rtmsg_ifinfo in the state machine lock, any change in
>> the state of slave could set a flag in the slave, it will indicated that
>> an rtmsg_ifinfo should be called at the end of the state machine.
> 
> 	To clarify, my opinion being referenced here was really asking
> Scott Feldman <sfeldma@cumulusnetworks.com> if: (a) the calls had to be
> synchronous, and, (b) if the intermediate calls to adjust flags within
> the ARP monitor "cycle through slaves looking for a functional slave"
> all required notifications.  My suspicion is that the answer to both of
> those is "no," but I haven't heard from Scott.
> 

Yes, this question has been in existence for a long time, and I admin your opinions, it is very
clear and reasonable, so I try to fix it by this way, in my original idea, I think not only
the 802.3ad state machine, but the ab, loadbalance monitor is still need to modify, the existing
solution for bond_ab_arp_probe which I think is not good enough, so I think it will be a big
patchset, so I send this patchset just only fix for 802.3ad for review.

>> Cc: Jay Vosburgh <fubar@us.ibm.com>
>> Cc: Veaceslav Falico <vfalico@redhat.com>
>> Cc: Andy Gospodarek <andy@greyhouse.net>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_3ad.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index cce1f1b..e80b78f 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -181,7 +181,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
>>  */
>> static inline void __disable_port(struct port *port)
>> {
>> -	bond_set_slave_inactive_flags(port->slave);
>> +	bond_set_slave_flags(port->slave, BOND_STATE_BACKUP, false);
>> }
>>
>> /**
>> @@ -193,7 +193,7 @@ static inline void __enable_port(struct port *port)
>> 	struct slave *slave = port->slave;
>>
>> 	if ((slave->link == BOND_LINK_UP) && IS_UP(slave->dev))
>> -		bond_set_slave_active_flags(slave);
>> +		bond_set_slave_flags(slave, BOND_STATE_ACTIVE, false);
> 
> 	I don't agree that we need to have two separate systems (your
> new bond_set_slave_flags plus bond_set_slave_{active,inactive}_flags)
> that both tweak the "active" or "inactive" flags for a slave.  It would
> be much cleaner and consistent with the current code to add a "notify"
> boolean to the existing functions.
> 
> 	-J

Yep, this problem has troubled me for a long time, whether to add a new function
or modify current function, I think your answer has gave me the right direction,
thanks.

Regards
Ding 

> 
>> }
>>
>> /**
>> @@ -2123,6 +2123,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>> re_arm:
>> 	rcu_read_unlock();
>> 	read_unlock(&bond->lock);
>> +	bond_slave_state_notify(bond, false);
>> 	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
>> }
>>
>> -- 
>> 1.8.0
> 
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH net-next 0/3] bonding: Fix RTNL: assertion failed at net/core/rtnetlink.c

[Thomas Glanzmann]

Hello Ding,

>   bonding: add bond_set_slave_state/flags()
>   bonding: add new slave param and  bond_slave_state_notify()
>   bonding: Fix the RTNL assertion failed for 802.3ad state machine

I applied the three patches on top of current Linus tip and recompiled,
my problems are gone. Find here dmesg and output of
/proc/net/bonding/bond?

(node-62) [~] dmesg | pbot
http://pbot.rmdir.de/pSIH_mE3PguPWsEYdVgusg
(node-62) [~] pbot /proc/net/bonding/bond0
http://pbot.rmdir.de/zNSSqmjSI0o1Qvt6DrSEQw
(node-62) [~] pbot /proc/net/bonding/bond1
http://pbot.rmdir.de/CoI00Rguie2P-kOQytOM9w

Tested-by: Thomas Glanzmann <thomas@glanzmann.de>

Cheers,
        Thomas

Re: [PATCH net-next 0/3] bonding: Fix RTNL: assertion failed at net/core/rtnetlink.c

[David Miller]

From: Ding Tianhong <dingtianhong@huawei.com>
Date: Mon, 17 Feb 2014 16:35:48 +0800

> The problem was introduced by the commit 1d3ee88ae0d
> (bonding: add netlink attributes to slave link dev).
> The bond_set_active_slave() and bond_set_backup_slave()
> will use rtmsg_ifinfo to send slave's states, so these
> two functions should be called in RTNL.
> 
> In 802.3ad mode, acquiring RTNL for the __enable_port and
> __disable_port cases is difficult, as those calls generally
> already hold the state machine lock, and cannot unconditionally
> call rtnl_lock because either they already hold RTNL (for calls
> via bond_3ad_unbind_slave) or due to the potential for deadlock
> with bond_3ad_adapter_speed_changed, bond_3ad_adapter_duplex_changed,
> bond_3ad_link_change, or bond_3ad_update_lacp_rate.  All four of
> those are called with RTNL held, and acquire the state machine lock
> second.  The calling contexts for __enable_port and __disable_port
> already hold the state machine lock, and may or may not need RTNL.
> 
> According to the Jay's opinion, I don't think it is a problem that
> the slave don't send notify message synchronously when the status
> changed, normally the state machine is running every 100 ms, send
> the notify message at the end of the state machine if the slave's
> state changed should be better.

I would like to ask you to combine these three changes into one
patch.

Thank you.