| Message ID | 1393427905-6811-5-git-send-email-vyasevic@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Feb 26, 2014 at 10:18:22AM -0500, Vlad Yasevich wrote: > When there is only 1 flooding port, this port is programmed > with all the address all the addresses? > the bridge accumulated. This allows > us to place this port into non-promiscuous mode. > At other times, all ports are set as promiscuous. To help > track whether the bridge set the mode or not, a new > flag is introduced. > > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > --- > net/bridge/br_if.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- > net/bridge/br_private.h | 1 + > 2 files changed, 47 insertions(+), 2 deletions(-) > > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > index e782c2e..51df642 100644 > --- a/net/bridge/br_if.c > +++ b/net/bridge/br_if.c > @@ -136,7 +136,7 @@ static void del_nbp(struct net_bridge_port *p) > > sysfs_remove_link(br->ifobj, p->dev->name); > > - dev_set_promiscuity(dev, -1); > + dev_set_allmulti(dev, -1); > > spin_lock_bh(&br->lock); > br_stp_disable_port(p); > @@ -359,7 +359,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) > > call_netdevice_notifiers(NETDEV_JOIN, dev); > > - err = dev_set_promiscuity(dev, 1); > + err = dev_set_allmulti(dev, 1); > if (err) > goto put_back; > > @@ -465,6 +465,48 @@ int br_del_if(struct net_bridge *br, struct net_device *dev) > return 0; > } > > +static int br_port_set_promisc(struct net_bridge_port *p) > +{ > + int err = 0; > + > + if (p->flags & BR_PROMISC) > + return err; > + > + err = dev_set_promiscuity(p->dev, 1); > + if (err) > + return err; > + > + p->flags |= BR_PROMISC; > + return err; > +} > + you take pains to return an error code here, only to ignore it in the caller? > +static void br_port_clear_promisc(struct net_bridge_port *p) > +{ > + if (!(p->flags & BR_PROMISC)) > + return; > + > + dev_set_promiscuity(p->dev, -1); > + p->flags &= ~BR_PROMISC; > +} > + > +/* When a port is added or removed or when the flooding status of > + * the port changes, this function is called to automatically mange > + * promiscuity setting of all the bridge ports. We are always called > + * under RTNL so can skip using rcu primitives. > + */ > +static void br_manage_promisc(struct net_bridge *br) > +{ > + struct net_bridge_port *p; > + > + list_for_each_entry(p, &br->port_list, list) { > + if (!br_port_exists(p->dev) || > + (br->n_flood_ports == 1 && br->c_flood_port == p)) > + br_port_clear_promisc(p); > + else > + br_port_set_promisc(p); > + } > +} > + > static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br) > { > /* Increment the number of flooding ports, and if we > @@ -475,6 +517,7 @@ static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br) > br->c_flood_port = p; > > br_fdb_addrs_sync(br); > + br_manage_promisc(br); > } > > static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br) > @@ -502,6 +545,7 @@ static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br) > } > } > } > + br_manage_promisc(br); > } > > void br_port_flags_change(struct net_bridge_port *p, unsigned long mask) > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 40a6927..6670cb3 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -174,6 +174,7 @@ struct net_bridge_port > #define BR_ADMIN_COST 0x00000010 > #define BR_LEARNING 0x00000020 > #define BR_FLOOD 0x00000040 > +#define BR_PROMISC 0x00000080 > Considering you then have a separate logic in 7/7 will this be cleaner as a per-port flag? > #ifdef CONFIG_BRIDGE_IGMP_SNOOPING > struct bridge_mcast_query ip4_query; > -- > 1.8.5.3 -- 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 02/26/2014 10:51 AM, Michael S. Tsirkin wrote: > On Wed, Feb 26, 2014 at 10:18:22AM -0500, Vlad Yasevich wrote: >> When there is only 1 flooding port, this port is programmed >> with all the address > > all the addresses? All statically configured ones plus locals. Locals are needed preserve the bridge behavior of being able to receive packets addresses to any of the ports mac addresses on any interface. This actually cuts the usability of this feature in about half. :( > >> the bridge accumulated. This allows >> us to place this port into non-promiscuous mode. >> At other times, all ports are set as promiscuous. To help >> track whether the bridge set the mode or not, a new >> flag is introduced. >> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >> --- >> net/bridge/br_if.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- >> net/bridge/br_private.h | 1 + >> 2 files changed, 47 insertions(+), 2 deletions(-) >> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index e782c2e..51df642 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -136,7 +136,7 @@ static void del_nbp(struct net_bridge_port *p) >> >> sysfs_remove_link(br->ifobj, p->dev->name); >> >> - dev_set_promiscuity(dev, -1); >> + dev_set_allmulti(dev, -1); >> >> spin_lock_bh(&br->lock); >> br_stp_disable_port(p); >> @@ -359,7 +359,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) >> >> call_netdevice_notifiers(NETDEV_JOIN, dev); >> >> - err = dev_set_promiscuity(dev, 1); >> + err = dev_set_allmulti(dev, 1); >> if (err) >> goto put_back; >> >> @@ -465,6 +465,48 @@ int br_del_if(struct net_bridge *br, struct net_device *dev) >> return 0; >> } >> >> +static int br_port_set_promisc(struct net_bridge_port *p) >> +{ >> + int err = 0; >> + >> + if (p->flags & BR_PROMISC) >> + return err; >> + >> + err = dev_set_promiscuity(p->dev, 1); >> + if (err) >> + return err; >> + >> + p->flags |= BR_PROMISC; >> + return err; >> +} >> + > > you take pains to return an error code here, > only to ignore it in the caller? You are right. This can be turned to void. > >> +static void br_port_clear_promisc(struct net_bridge_port *p) >> +{ >> + if (!(p->flags & BR_PROMISC)) >> + return; >> + >> + dev_set_promiscuity(p->dev, -1); >> + p->flags &= ~BR_PROMISC; >> +} >> + >> +/* When a port is added or removed or when the flooding status of >> + * the port changes, this function is called to automatically mange >> + * promiscuity setting of all the bridge ports. We are always called >> + * under RTNL so can skip using rcu primitives. >> + */ >> +static void br_manage_promisc(struct net_bridge *br) >> +{ >> + struct net_bridge_port *p; >> + >> + list_for_each_entry(p, &br->port_list, list) { >> + if (!br_port_exists(p->dev) || >> + (br->n_flood_ports == 1 && br->c_flood_port == p)) >> + br_port_clear_promisc(p); >> + else >> + br_port_set_promisc(p); >> + } >> +} >> + >> static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br) >> { >> /* Increment the number of flooding ports, and if we >> @@ -475,6 +517,7 @@ static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br) >> br->c_flood_port = p; >> >> br_fdb_addrs_sync(br); >> + br_manage_promisc(br); >> } >> >> static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br) >> @@ -502,6 +545,7 @@ static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br) >> } >> } >> } >> + br_manage_promisc(br); >> } >> >> void br_port_flags_change(struct net_bridge_port *p, unsigned long mask) >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >> index 40a6927..6670cb3 100644 >> --- a/net/bridge/br_private.h >> +++ b/net/bridge/br_private.h >> @@ -174,6 +174,7 @@ struct net_bridge_port >> #define BR_ADMIN_COST 0x00000010 >> #define BR_LEARNING 0x00000020 >> #define BR_FLOOD 0x00000040 >> +#define BR_PROMISC 0x00000080 >> > > Considering you then have a separate logic in 7/7 > will this be cleaner as a per-port flag? It is a per-port flag here. I can change it to BR_PORT_PROMISC to make it clearer. -vlad > >> #ifdef CONFIG_BRIDGE_IGMP_SNOOPING >> struct bridge_mcast_query ip4_query; >> -- >> 1.8.5.3 -- 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, 26 Feb 2014 10:18:22 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote: > When there is only 1 flooding port, this port is programmed > with all the address the bridge accumulated. This allows > us to place this port into non-promiscuous mode. > At other times, all ports are set as promiscuous. To help > track whether the bridge set the mode or not, a new > flag is introduced. > > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> This mixes the definition of outbound (flooding) and inbound (promiscuous). Not sure if this is safe in all cases. -- 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, Feb 26, 2014 at 08:58:26AM -0800, Stephen Hemminger wrote: > On Wed, 26 Feb 2014 10:18:22 -0500 > Vlad Yasevich <vyasevic@redhat.com> wrote: > > > When there is only 1 flooding port, this port is programmed > > with all the address the bridge accumulated. This allows > > us to place this port into non-promiscuous mode. > > At other times, all ports are set as promiscuous. To help > > track whether the bridge set the mode or not, a new > > flag is introduced. > > > > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > > This mixes the definition of outbound (flooding) and inbound (promiscuous). > Not sure if this is safe in all cases. Logically: inbound on port A == outbound on all ports except A So promisc on A == OR of flood on all ports except A This rule should just be applied to all ports. Makes sense, right? I think this is what this tries to implement, even if the optimization attempt masks the logic somewhat. -- 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
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c index e782c2e..51df642 100644 --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -136,7 +136,7 @@ static void del_nbp(struct net_bridge_port *p) sysfs_remove_link(br->ifobj, p->dev->name); - dev_set_promiscuity(dev, -1); + dev_set_allmulti(dev, -1); spin_lock_bh(&br->lock); br_stp_disable_port(p); @@ -359,7 +359,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) call_netdevice_notifiers(NETDEV_JOIN, dev); - err = dev_set_promiscuity(dev, 1); + err = dev_set_allmulti(dev, 1); if (err) goto put_back; @@ -465,6 +465,48 @@ int br_del_if(struct net_bridge *br, struct net_device *dev) return 0; } +static int br_port_set_promisc(struct net_bridge_port *p) +{ + int err = 0; + + if (p->flags & BR_PROMISC) + return err; + + err = dev_set_promiscuity(p->dev, 1); + if (err) + return err; + + p->flags |= BR_PROMISC; + return err; +} + +static void br_port_clear_promisc(struct net_bridge_port *p) +{ + if (!(p->flags & BR_PROMISC)) + return; + + dev_set_promiscuity(p->dev, -1); + p->flags &= ~BR_PROMISC; +} + +/* When a port is added or removed or when the flooding status of + * the port changes, this function is called to automatically mange + * promiscuity setting of all the bridge ports. We are always called + * under RTNL so can skip using rcu primitives. + */ +static void br_manage_promisc(struct net_bridge *br) +{ + struct net_bridge_port *p; + + list_for_each_entry(p, &br->port_list, list) { + if (!br_port_exists(p->dev) || + (br->n_flood_ports == 1 && br->c_flood_port == p)) + br_port_clear_promisc(p); + else + br_port_set_promisc(p); + } +} + static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br) { /* Increment the number of flooding ports, and if we @@ -475,6 +517,7 @@ static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br) br->c_flood_port = p; br_fdb_addrs_sync(br); + br_manage_promisc(br); } static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br) @@ -502,6 +545,7 @@ static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br) } } } + br_manage_promisc(br); } void br_port_flags_change(struct net_bridge_port *p, unsigned long mask) diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 40a6927..6670cb3 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -174,6 +174,7 @@ struct net_bridge_port #define BR_ADMIN_COST 0x00000010 #define BR_LEARNING 0x00000020 #define BR_FLOOD 0x00000040 +#define BR_PROMISC 0x00000080 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING struct bridge_mcast_query ip4_query;
When there is only 1 flooding port, this port is programmed with all the address the bridge accumulated. This allows us to place this port into non-promiscuous mode. At other times, all ports are set as promiscuous. To help track whether the bridge set the mode or not, a new flag is introduced. Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> --- net/bridge/br_if.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++-- net/bridge/br_private.h | 1 + 2 files changed, 47 insertions(+), 2 deletions(-)