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

[Vlad Yasevich <vyasevic@redhat.com>]

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

>> ---
>>  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