Re: [PATCH 7/7] bridge: Support promisc management when all ports are non-flooding

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Feb 26, 2014 at 10:46:20PM -0500, Vlad Yasevich wrote:
> On 02/26/2014 10:57 AM, Michael S. Tsirkin wrote:
> > On Wed, Feb 26, 2014 at 10:18:25AM -0500, Vlad Yasevich wrote:
> >> Configuration where all ports are set to non-flooding is a slight
> >> special case.  In this config, the user would have to manage all fdbs
> >> for all ports.  In this special case, since we'll know all addresses,
> >> we can turn off promisc on all ports and program all ports with the
> >> same set of addresses.
> >>
> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> > 
> > Here the logic is different be we could sync them,
> > just make this a per-port flag and
> > scan all ports instead of relying on flood_port.
> > This will be slower in the single flooding port
> > case but worst case remains the same since
> > this patch scans all ports.
> > 
> > What do you think?
> 
> I had it this way before and didn't really like it.  The transitions
> between the two configs needs to be detected and appropriate
> actions taken.  So it'll still be special coded.
> 
> I'll see what I can do to simplify this.
> 
> -vlad

Not really.
Basically if you have per-port flags p->promisc and p->flood
then the logic becomes:

	int nr_flood = 0;
	for each port (br, p) {
		nr_flood += p->flood;
	}

	for each port (br, p) {
		int promisc = nr_flood - p->flood;
		if (p->promisc != promisc) {
			dev_set_promisc(p, promisc);
			dev_set_allmulti(p, !promisc);
		}
	}

Just re-run this whenever user tweaks any flood flags.

Replace p->flood with p->flood && p->learn if you
need to disable the optimization when learning
is on.

> >> ---
> >>  include/linux/netdevice.h |  3 +++
> >>  net/bridge/br_fdb.c       | 22 +++++++++++++++++-----
> >>  net/bridge/br_if.c        | 40 ++++++++++++++++++++++++++++++++++------
> >>  net/bridge/br_private.h   |  2 +-
> >>  net/core/dev_addr_lists.c |  7 ++++---
> >>  5 files changed, 59 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index e29cce1..79e97ee 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -2889,6 +2889,9 @@ int __hw_addr_del(struct netdev_hw_addr_list *list,
> >>  		  unsigned char addr_type);
> >>  int __hw_addr_sync(struct netdev_hw_addr_list *to_list,
> >>  		   struct netdev_hw_addr_list *from_list, int addr_len);
> >> +int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> >> +			    struct netdev_hw_addr_list *from_list,
> >> +			    int addr_len);
> >>  void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
> >>  		      struct netdev_hw_addr_list *from_list, int addr_len);
> >>  void __hw_addr_init(struct netdev_hw_addr_list *list);
> >> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> >> index ef95e81..26ea4fe 100644
> >> --- a/net/bridge/br_fdb.c
> >> +++ b/net/bridge/br_fdb.c
> >> @@ -911,19 +911,31 @@ void br_fdb_addrs_sync(struct net_bridge *br)
> >>  	if (br->n_flood_ports == 1) {
> >>  		p = br->c_flood_port;
> >>  		netif_addr_lock(p->dev);
> >> -		err = __hw_addr_sync(&p->dev->uc, &br->conf_addrs,
> >> -				     p->dev->addr_len);
> >> +		err = __hw_addr_sync_multiple(&p->dev->uc, &br->conf_addrs,
> >> +					      p->dev->addr_len);
> >>  		if (!err)
> >>  			__dev_set_rx_mode(p->dev);
> >>  		netif_addr_unlock(p->dev);
> >> +	} else if (br->n_flood_ports == 0) {
> >> +		list_for_each_entry(p, &br->port_list, list) {
> >> +			/* skip over ports being deleted. */
> >> +			if (!br_port_exists(p->dev))
> >> +				continue;
> >>  
> >> +			netif_addr_lock_nested(p->dev);
> >> +			err = __hw_addr_sync_multiple(&p->dev->uc,
> >> +						      &br->conf_addrs,
> >> +						      p->dev->addr_len);
> >> +			if (!err)
> >> +				__dev_set_rx_mode(p->dev);
> >> +			netif_addr_unlock(p->dev);
> >> +		}
> >>  	}
> >> +
> >>  }
> >>  
> >> -void br_fdb_addrs_unsync(struct net_bridge *br)
> >> +void br_fdb_addrs_unsync(struct net_bridge *br, struct net_bridge_port *p)
> >>  {
> >> -	struct net_bridge_port *p = br->c_flood_port;
> >> -
> >>  	if (!p)
> >>  		return;
> >>  
> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> >> index 55e4e28..4ba62ef 100644
> >> --- a/net/bridge/br_if.c
> >> +++ b/net/bridge/br_if.c
> >> @@ -148,6 +148,8 @@ static void del_nbp(struct net_bridge_port *p)
> >>  
> >>  	if (p->flags & BR_FLOOD)
> >>  		br_del_flood_port(p, br);
> >> +	else
> >> +		br_fdb_addrs_unsync(br, p);
> >>  
> >>  	nbp_vlan_flush(p);
> >>  	br_fdb_delete_by_port(br, p, 1);
> >> @@ -530,8 +532,26 @@ static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br)
> >>  	 * only have 1 flooding port cache if for future use.
> >>  	 */
> >>  	br->n_flood_ports++;
> >> -	if (br->n_flood_ports == 1)
> >> +	if (br->n_flood_ports == 1) {
> >> +		struct net_bridge_port *port;
> >> +
> >> +		/* We are transitioning from 0 flood ports to 1.  Remove
> >> +		 * the addresses from all the non-flood ports and turn on
> >> +		 * promisc on those ports.
> >> +		 */
> >> +		list_for_each_entry(port, &br->port_list, list) {
> >> +			/* skip the current port we are changing */
> >> +			if (port == p)
> >> +				continue;
> >> +
> >> +			if (!(port->flags & BR_FLOOD)) {
> >> +				br_port_set_promisc(port);
> >> +				br_fdb_addrs_unsync(br, port);
> >> +			}
> >> +		}
> >> +
> >>  		br->c_flood_port = p;
> >> +	}
> >>  
> >>  	br_fdb_addrs_sync(br);
> >>  	br_manage_promisc(br);
> >> @@ -547,12 +567,20 @@ static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br)
> >>  	 * set it if it is not set.
> >>  	 */
> >>  	br->n_flood_ports--;
> >> -	if (p == br->c_flood_port) {
> >> -		br_fdb_addrs_unsync(br);
> >> -		br->c_flood_port = NULL;
> >> -	}
> >> +	if (br->n_flood_ports == 0) {
> >> +		/* We just dropped to 0 flood ports.  If we
> >> +		 * are deleting this port, unsync addresses
> >> +		 * from it.
> >> +		 */
> >> +		if (!br_port_exists(p->dev))
> >> +			br_fdb_addrs_unsync(br, p);
> >>  
> >> -	if (br->n_flood_ports == 1) {
> >> +		br->c_flood_port = NULL;
> >> +		br_fdb_addrs_sync(br);
> >> +	} else if (br->n_flood_ports == 1) {
> >> +		/* We just dropped to 1 flood port. Find this one flood
> >> +		 * port and sync to it.
> >> +		 */
> >>  		list_for_each_entry(port, &p->br->port_list, list) {
> >>  			if (br_port_exists(port->dev) &&
> >>  			    (port->flags & BR_FLOOD)) {
> >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> >> index 87dcc09..13840de 100644
> >> --- a/net/bridge/br_private.h
> >> +++ b/net/bridge/br_private.h
> >> @@ -400,7 +400,7 @@ int br_fdb_add(struct ndmsg *nlh, struct nlattr *tb[], struct net_device *dev,
> >>  int br_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
> >>  		struct net_device *dev, int idx);
> >>  void br_fdb_addrs_sync(struct net_bridge *br);
> >> -void br_fdb_addrs_unsync(struct net_bridge *br);
> >> +void br_fdb_addrs_unsync(struct net_bridge *br, struct net_bridge_port *p);
> >>  
> >>  /* br_forward.c */
> >>  void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
> >> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> >> index 3de44a3..24da78f 100644
> >> --- a/net/core/dev_addr_lists.c
> >> +++ b/net/core/dev_addr_lists.c
> >> @@ -171,9 +171,9 @@ static void __hw_addr_unsync_one(struct netdev_hw_addr_list *to_list,
> >>  	__hw_addr_del_entry(from_list, ha, false, false);
> >>  }
> >>  
> >> -static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> >> -				   struct netdev_hw_addr_list *from_list,
> >> -				   int addr_len)
> >> +int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> >> +			    struct netdev_hw_addr_list *from_list,
> >> +			    int addr_len)
> >>  {
> >>  	int err = 0;
> >>  	struct netdev_hw_addr *ha, *tmp;
> >> @@ -189,6 +189,7 @@ static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
> >>  	}
> >>  	return err;
> >>  }
> >> +EXPORT_SYMBOL(__hw_addr_sync_multiple);
> >>  
> >>  /* This function only works where there is a strict 1-1 relationship
> >>   * between source and destionation of they synch. If you ever need to
> >> -- 
> >> 1.8.5.3