Patchwork [1/2] Issue NETDEV_CHANGE notification when bridge changes state

login
register
mail settings
Submitter Adam Majer
Date March 6, 2011, 5:18 a.m.
Message ID <20110306051833.GA3098@mira.lan.galacticasoftware.com>
Download mbox | patch
Permalink /patch/85572/
State Superseded
Delegated to: David Miller
Headers show

Comments

Adam Majer - March 6, 2011, 5:18 a.m.
IPv6 address autoconfiguration relies on an UP interface to processes
traffic normally. A bridge that is UP enters LEARNING state and this
state "eats" any Router Advertising packets sent to the
bridge. Issuing a NETDEV_CHANGE notification on the bridge interface
when it changes state allows autoconfiguration code to retry
querying router information.

Signed-off-by: Adam Majer <adamm@zombino.com>
---
 net/bridge/br_if.c      |   20 ++++++++++++++++++++
 net/bridge/br_private.h |    2 ++
 net/bridge/br_stp.c     |    2 ++
 3 files changed, 24 insertions(+), 0 deletions(-)
Stephen Hemminger - March 6, 2011, 6:43 a.m.
Why not set forwarding delay to zero? I don't think you are using STP?

P.s: removing linux-kernel mailing list, since there is no point
in copying the whole world on this thread.
--
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
Adam Majer - March 6, 2011, 8:03 a.m.
On Sat, Mar 05, 2011 at 10:43:03PM -0800, Stephen Hemminger wrote:
> Why not set forwarding delay to zero? I don't think you are using STP?
> 
> P.s: removing linux-kernel mailing list, since there is no point
> in copying the whole world on this thread.

I just sent patches where MAINTAINERS file told me to send it. ;)

No, I'm not relying on STP. I think setting learning->forwarding delay
to 0 could cause problems with STP like loops until the loop is
detected.

There may be a better spot where to insert the notification that the
bridge is forwarding data, though I'm not exactly certain where. The
patch will cause bridge to issue NETDEV_CHANGE notification for all
topology changes. This may or may not be useful but shouldn't be
harmful (I can't imagine these occur very often?) The IPv6 autoconf
patch will only act on this if it doesn't have IPv6 address configured
manually or via autoconf.
stephen hemminger - March 6, 2011, 5:45 p.m.
On Sun, 6 Mar 2011 02:03:28 -0600
Adam Majer <adamm@zombino.com> wrote:

> On Sat, Mar 05, 2011 at 10:43:03PM -0800, Stephen Hemminger wrote:
> > Why not set forwarding delay to zero? I don't think you are using STP?
> > 
> > P.s: removing linux-kernel mailing list, since there is no point
> > in copying the whole world on this thread.
> 
> I just sent patches where MAINTAINERS file told me to send it. ;)
> 
> No, I'm not relying on STP. I think setting learning->forwarding delay
> to 0 could cause problems with STP like loops until the loop is
> detected.
> 
> There may be a better spot where to insert the notification that the
> bridge is forwarding data, though I'm not exactly certain where. The
> patch will cause bridge to issue NETDEV_CHANGE notification for all
> topology changes. This may or may not be useful but shouldn't be
> harmful (I can't imagine these occur very often?) The IPv6 autoconf
> patch will only act on this if it doesn't have IPv6 address configured
> manually or via autoconf.
> 

Since this a generic problem, it needs a better solution.
Sending NETDEV_CHANGE impacts lots of other pieces, and even
user space has similar problems.
Jan Ceuleers - March 6, 2011, 6:01 p.m.
On 06/03/11 09:03, Adam Majer wrote:
> On Sat, Mar 05, 2011 at 10:43:03PM -0800, Stephen Hemminger wrote:
>> Why not set forwarding delay to zero? I don't think you are using STP?
 >
> No, I'm not relying on STP. I think setting learning->forwarding delay
> to 0 could cause problems with STP like loops until the loop is
> detected.

I don't think that Stephen is saying that you should hard-code the 
forwarding delay as zero (i.e. to remove the delay from the kernel 
completely), but rather use brctl to set the forwarding delay to zero on 
bridge instances in which you're not using STP.

HTH, Jan

--
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
Adam Majer - March 7, 2011, 12:25 a.m.
On Sun, Mar 06, 2011 at 09:45:41AM -0800, Stephen Hemminger wrote:
> Since this a generic problem, it needs a better solution.
> Sending NETDEV_CHANGE impacts lots of other pieces, and even
> user space has similar problems.

It does seem a little broad notification type. I've checked over
all the currently defined NETDEV notifiers, and it seems that
NETDEV_NOTIFY_PEERS may be a better option to use when bridge
has a potential topology change.

Currently it is only used in ipv4/devinet.c: where it is used to issue
a gratuitous ARP.
Stephen Hemminger - March 7, 2011, 6:41 a.m.
> On Sun, Mar 06, 2011 at 09:45:41AM -0800, Stephen Hemminger wrote:
> > Since this a generic problem, it needs a better solution.
> > Sending NETDEV_CHANGE impacts lots of other pieces, and even
> > user space has similar problems.
> 
> It does seem a little broad notification type. I've checked over
> all the currently defined NETDEV notifiers, and it seems that
> NETDEV_NOTIFY_PEERS may be a better option to use when bridge
> has a potential topology change.
> 
> Currently it is only used in ipv4/devinet.c: where it is used to issue
> a gratuitous ARP.

I was thinking of fixing bridge to not actually bring the link
up until in forwarding mode. Other applications (DHCP, etc)
see the link up and really don't like being in half duplex
during that period. 
--
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
Nicolas de Pesloüan - March 7, 2011, 7:44 a.m.
Le 07/03/2011 07:41, Stephen Hemminger a écrit :
>
>> On Sun, Mar 06, 2011 at 09:45:41AM -0800, Stephen Hemminger wrote:
>>> Since this a generic problem, it needs a better solution.
>>> Sending NETDEV_CHANGE impacts lots of other pieces, and even
>>> user space has similar problems.
>>
>> It does seem a little broad notification type. I've checked over
>> all the currently defined NETDEV notifiers, and it seems that
>> NETDEV_NOTIFY_PEERS may be a better option to use when bridge
>> has a potential topology change.
>>
>> Currently it is only used in ipv4/devinet.c: where it is used to issue
>> a gratuitous ARP.
>
> I was thinking of fixing bridge to not actually bring the link
> up until in forwarding mode. Other applications (DHCP, etc)
> see the link up and really don't like being in half duplex
> during that period.

I think it is the right way to manage this situation. And bonding should behave the same, if not 
already true.

	Nicolas.
--
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
WANG Cong - March 9, 2011, 3:09 p.m.
On Sat, Mar 05, 2011 at 11:18:33PM -0600, Adam Majer wrote:
>
>IPv6 address autoconfiguration relies on an UP interface to processes
>traffic normally. A bridge that is UP enters LEARNING state and this
>state "eats" any Router Advertising packets sent to the
>bridge. Issuing a NETDEV_CHANGE notification on the bridge interface
>when it changes state allows autoconfiguration code to retry
>querying router information.
>
...

>+static void br_port_change_notifier_handler(struct work_struct *work)
>+{
>+	struct net_bridge *br = container_of(work,
>+					     struct net_bridge,
>+					     change_notification_worker);
>+
>+	rtnl_lock();
>+	netdev_state_change(br->dev);
>+	rtnl_unlock();
>+}
>+

Do you really want user-space to get this notification too?
Why do you put it into a workqueue? Maybe it has to be called in
process-context?

Thanks.
--
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
Adam Majer - March 9, 2011, 4:44 p.m.
On Wed, Mar 09, 2011 at 11:09:49PM +0800, Américo Wang wrote:
> On Sat, Mar 05, 2011 at 11:18:33PM -0600, Adam Majer wrote:
> >
> >IPv6 address autoconfiguration relies on an UP interface to processes
> >traffic normally. A bridge that is UP enters LEARNING state and this
> >state "eats" any Router Advertising packets sent to the
> >bridge. Issuing a NETDEV_CHANGE notification on the bridge interface
> >when it changes state allows autoconfiguration code to retry
> >querying router information.
> >
> ...
> 
> >+static void br_port_change_notifier_handler(struct work_struct *work)
> >+{
> >+	struct net_bridge *br = container_of(work,
> >+					     struct net_bridge,
> >+					     change_notification_worker);
> >+
> >+	rtnl_lock();
> >+	netdev_state_change(br->dev);
> >+	rtnl_unlock();
> >+}
> >+
> 
> Do you really want user-space to get this notification too?
> Why do you put it into a workqueue? Maybe it has to be called in
> process-context?

Stephen Hemminger's patch (posted as a reply in this thread but only
on the netdev mailing list) is much better as it only brings the link
up on the bridge interface when the bridge enters forwarding mode.

Effectively it does similar thing via NETDEV_CHANGE but requires no
other hacks in other code. It should make userland much happier too.


As for my patch, I've put that in the workqueue because the
notification cannot be called in interrupt context. Is there a better
way of calling code that can sleep from interrupt context?

- Adam

Patch

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..9604d52 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -25,6 +25,22 @@ 
 
 #include "br_private.h"
 
+
+/*
+ * Notifies that the bridge has changed state
+ */
+static void br_port_change_notifier_handler(struct work_struct *work)
+{
+	struct net_bridge *br = container_of(work,
+					     struct net_bridge,
+					     change_notification_worker);
+
+	rtnl_lock();
+	netdev_state_change(br->dev);
+	rtnl_unlock();
+}
+
+
 /*
  * Determine initial path cost based on speed.
  * using recommendations from 802.1d standard
@@ -163,6 +179,7 @@  static void del_br(struct net_bridge *br, struct list_head *head)
 {
 	struct net_bridge_port *p, *n;
 
+	flush_work(&br->change_notification_worker);
 	list_for_each_entry_safe(p, n, &br->port_list, list) {
 		del_nbp(p);
 	}
@@ -203,6 +220,9 @@  static struct net_device *new_bridge_dev(struct net *net, const char *name)
 
 	memcpy(br->group_addr, br_group_address, ETH_ALEN);
 
+	INIT_WORK(&br->change_notification_worker,
+		  &br_port_change_notifier_handler);
+
 	br->feature_mask = dev->features;
 	br->stp_enabled = BR_NO_STP;
 	br->designated_root = br->bridge_id;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4e1b620..7f8ef41 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -179,6 +179,8 @@  struct net_bridge
 	struct list_head		port_list;
 	struct net_device		*dev;
 
+	struct work_struct		change_notification_worker;
+
 	struct br_cpu_netstats __percpu *stats;
 	spinlock_t			hash_lock;
 	struct hlist_head		hash[BR_HASH_SIZE];
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 57186d8..b5be3e3 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -296,6 +296,8 @@  void br_topology_change_detection(struct net_bridge *br)
 {
 	int isroot = br_is_root_bridge(br);
 
+	queue_work(system_long_wq, &br->change_notification_worker);
+
 	if (br->stp_enabled != BR_KERNEL_STP)
 		return;