| Message ID | 20110306051833.GA3098@mira.lan.galacticasoftware.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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.
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.
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
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.
> 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
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
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
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
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;
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(-)