Re: [PATCH net-next 3/4] bridge: support binding vlan dev link state to vlan member bridge ports

[Nikolay Aleksandrov <nikolay@cumulusnetworks.com>]

On 02/04/2019 18:35, Mike Manning wrote:
> In the case of vlan filtering on bridges, the bridge may also have the
> corresponding vlan devices as upper devices. A vlan bridge binding mode
> is added to allow the link state of the vlan device to track only the
> state of the subset of bridge ports that are also members of the vlan,
> rather than that of all bridge ports. This mode is set with a vlan flag
> rather than a bridge sysfs so that the 8021q module is aware that it
> should not set the link state for the vlan device.
> 
> If bridge vlan is configured, the bridge device event handling results
> in the link state for an upper device being set, if it is a vlan device
> with the vlan bridge binding mode enabled. This also sets a
> vlan_bridge_binding flag so that subsequent UP/DOWN/CHANGE events for
> the ports in that bridge result in a link state update of the vlan
> device if required.
> 
> The link state of the vlan device is up if there is at least one bridge
> port that is a vlan member that is admin & oper up, otherwise its oper
> state is IF_OPER_LOWERLAYERDOWN.
> 
> Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
> ---
>  net/bridge/br.c         |  23 ++++++--
>  net/bridge/br_private.h |  17 ++++++
>  net/bridge/br_vlan.c    | 143 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 179 insertions(+), 4 deletions(-)
> 

Hi,
Please CC bridge maintainers when sending bridge patches.
One question/thought - can't we add a ports_up counter in the vlan's master
struct and keep how many ports are up for that vlan ?
The important part would be to keep it correct, i.e. vlan_add/del should inc/dec
as well as port up/down. Then we can directly update its carrier on port event
without doing a possible O(n^2) walk, we just need to walk over the port vlans
and adjust counters which is always O(n) based on num of that port's vlans.

Some more comments below.


> diff --git a/net/bridge/br.c b/net/bridge/br.c
> index a5174e5001d8..b80cd5ccd590 100644
> --- a/net/bridge/br.c
> +++ b/net/bridge/br.c
> @@ -40,10 +40,21 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
>  	bool changed_addr;
>  	int err;
>  
> -	/* register of bridge completed, add sysfs entries */
> -	if ((dev->priv_flags & IFF_EBRIDGE) && event == NETDEV_REGISTER) {
> -		br_sysfs_addbr(dev);
> -		return NOTIFY_DONE;
> +	if (dev->priv_flags & IFF_EBRIDGE) {
> +		if (event == NETDEV_REGISTER) {
> +			/* register of bridge completed, add sysfs entries */
> +			br_sysfs_addbr(dev);
> +			return NOTIFY_DONE;
> +		}
> +#ifdef CONFIG_BRIDGE_VLAN_FILTERING
> +		if (event == NETDEV_CHANGEUPPER) {
> +			struct netdev_notifier_changeupper_info *info = ptr;
> +
> +			br_vlan_upper_change(dev, info->upper_dev,
> +					     info->linking);
> +			return NOTIFY_DONE;
> +		}
> +#endif
>  	}
>  
>  	/* not a port of a bridge */
> @@ -126,6 +137,10 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
>  		break;
>  	}
>  
> +#ifdef CONFIG_BRIDGE_VLAN_FILTERING
> +	br_vlan_port_event(p, br, event);
> +#endif
> +
>  	/* Events that may cause spanning tree to refresh */
>  	if (!notified && (event == NETDEV_CHANGEADDR || event == NETDEV_UP ||
>  			  event == NETDEV_CHANGE || event == NETDEV_DOWN))
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 00deef7fc1f3..604de174abe0 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -336,6 +336,7 @@ struct net_bridge {
>  #ifdef CONFIG_BRIDGE_VLAN_FILTERING
>  	__be16				vlan_proto;
>  	u16				default_pvid;
> +	u8				vlan_bridge_binding;

Use the bridge private bit options for this, don't add new fields. Take a look
at the br_opt_get/br_opt_toggle and the BROPT_ options.

>  	struct net_bridge_vlan_group	__rcu *vlgrp;
>  #endif
>  
> @@ -896,6 +897,10 @@ int nbp_vlan_init(struct net_bridge_port *port, struct netlink_ext_ack *extack);
>  int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
>  void br_vlan_get_stats(const struct net_bridge_vlan *v,
>  		       struct br_vlan_stats *stats);
> +void br_vlan_port_event(struct net_bridge_port *p, struct net_bridge *br,
> +			unsigned long event);
> +void br_vlan_upper_change(struct net_device *dev, struct net_device *upper_dev,
> +			  bool linking);
>  
>  static inline struct net_bridge_vlan_group *br_vlan_group(
>  					const struct net_bridge *br)
> @@ -1079,6 +1084,18 @@ static inline void br_vlan_get_stats(const struct net_bridge_vlan *v,
>  				     struct br_vlan_stats *stats)
>  {
>  }
> +
> +static inline void br_vlan_port_event(struct net_bridge_port *p,
> +				      struct net_bridge *br,
> +				      unsigned long event)
> +{
> +}
> +
> +static inline void br_vlan_upper_change(struct net_device *dev,
> +					struct net_device *upper_dev,
> +					bool linking)
> +{
> +}
>  #endif
>  
>  struct nf_br_ops {
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 96abf8feb9dc..642373231386 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -1265,3 +1265,146 @@ int br_vlan_get_info(const struct net_device *dev, u16 vid,
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(br_vlan_get_info);
> +
> +static int br_vlan_is_bind_vlan_dev(struct net_device *dev)

const dev

> +{
> +	return is_vlan_dev(dev) &&
> +		!!(vlan_dev_priv(dev)->flags & VLAN_FLAG_BRIDGE_BINDING);
> +}
> +
> +static int br_vlan_is_bind_vlan_dev_fn(struct net_device *dev,
> +				       __always_unused void *data)
> +{
> +	return br_vlan_is_bind_vlan_dev(dev);
> +}
> +
> +static int br_vlan_has_upper_bind_vlan_dev(struct net_device *dev)
> +{
> +	int found;
> +
> +	rcu_read_lock();
> +	found = netdev_walk_all_upper_dev_rcu(dev, br_vlan_is_bind_vlan_dev_fn,
> +					      NULL);
> +	rcu_read_unlock();
> +
> +	return found;
> +}
> +
> +struct br_vlan_bind_walk_data {
> +	u16 vid;
> +	struct net_device *result;
> +};
> +
> +static int br_vlan_match_bind_vlan_dev_fn(struct net_device *dev, void *data_in)
> +{
> +	struct br_vlan_bind_walk_data *data = data_in;
> +	int found = 0;
> +
> +	if (br_vlan_is_bind_vlan_dev(dev) &&
> +	    vlan_dev_priv(dev)->vlan_id == data->vid) {
> +		dev_hold(dev);

Why do you need dev_hold() ?
This seems to be running under rtnl.

> +		data->result = dev;
> +		found = 1;
> +	}
> +
> +	return found;
> +}
> +
> +/* If found, returns the vlan device with a reference held, else returns NULL.
> + */
> +static struct net_device *
> +br_vlan_get_upper_bind_vlan_dev(struct net_device *dev, u16 vid)
> +{
> +	struct br_vlan_bind_walk_data data = {
> +		.vid = vid,
> +	};
> +
> +	rcu_read_lock();
> +	netdev_walk_all_upper_dev_rcu(dev, br_vlan_match_bind_vlan_dev_fn,
> +				      &data);
> +	rcu_read_unlock();
> +
> +	return data.result;
> +}
> +
> +static bool br_vlan_is_dev_up(struct net_device *dev)

const dev

> +{
> +	return  !!(dev->flags & IFF_UP) && netif_oper_up(dev);
> +}
> +
> +static void br_vlan_set_vlan_dev_state(struct net_bridge *br,
> +				       struct net_device *vlan_dev)
> +{
> +	u16 vid = vlan_dev_priv(vlan_dev)->vlan_id;
> +	struct net_bridge_vlan_group *vg;
> +	struct net_bridge_port *p;
> +	bool has_carrier = false;
> +
> +	list_for_each_entry(p, &br->port_list, list) {
> +		vg = nbp_vlan_group(p);
> +		if (br_vlan_find(vg, vid) && br_vlan_is_dev_up(p->dev)) {
> +			has_carrier = true;
> +			break;
> +		}
> +	}
> +
> +	if (netif_carrier_ok(vlan_dev)) {
> +		if (!has_carrier)
> +			netif_carrier_off(vlan_dev);
> +	} else {
> +		if (has_carrier)
> +			netif_carrier_on(vlan_dev);
> +	}
> +}
> +
> +static void br_vlan_set_all_vlan_dev_state(struct net_bridge_port *p,
> +					   struct net_bridge *br)

br is redundant, you can access it via p->br, you can define it locally
if needed

> +{
> +	struct net_bridge_vlan_group *vg = nbp_vlan_group(p);
> +	struct net_bridge_vlan *vlan;
> +	struct net_device *vlan_dev;
> +
> +	list_for_each_entry(vlan, &vg->vlan_list, vlist) {
> +		vlan_dev = br_vlan_get_upper_bind_vlan_dev(br->dev, vlan->vid);
> +		if (vlan_dev) {
> +			if (br_vlan_is_dev_up(p->dev)) {
> +				if (!netif_carrier_ok(vlan_dev))
> +					netif_carrier_on(vlan_dev);
> +			} else {
> +				br_vlan_set_vlan_dev_state(br, vlan_dev);
> +			}
> +			dev_put(vlan_dev);
> +		}
> +	}
> +}
> +
> +void br_vlan_upper_change(struct net_device *dev, struct net_device *upper_dev,
> +			  bool linking)
> +{
> +	struct net_bridge *br = netdev_priv(dev);
> +
> +	if (!br_vlan_is_bind_vlan_dev(upper_dev))
> +		return;
> +
> +	if (linking) {
> +		br_vlan_set_vlan_dev_state(br, upper_dev);
> +		br->vlan_bridge_binding = 1;
> +	} else {
> +		br->vlan_bridge_binding = br_vlan_has_upper_bind_vlan_dev(dev);
> +	}
> +}
> +
> +void br_vlan_port_event(struct net_bridge_port *p, struct net_bridge *br,
> +			unsigned long event)

br is redundant, p->br is available

> +{
> +	if (!br->vlan_bridge_binding)
> +		return;
> +
> +	switch (event) {
> +	case NETDEV_CHANGE:
> +	case NETDEV_DOWN:
> +	case NETDEV_UP:
> +		br_vlan_set_all_vlan_dev_state(p, br);
> +		break;
> +	}
> +}
>