From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3B8E4C4360F for ; Tue, 2 Apr 2019 19:22:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EA34C2082C for ; Tue, 2 Apr 2019 19:22:16 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=cumulusnetworks.com header.i=@cumulusnetworks.com header.b="dW7jOUDC" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729400AbfDBTWP (ORCPT ); Tue, 2 Apr 2019 15:22:15 -0400 Received: from mail-wr1-f68.google.com ([209.85.221.68]:32879 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726083AbfDBTWP (ORCPT ); Tue, 2 Apr 2019 15:22:15 -0400 Received: by mail-wr1-f68.google.com with SMTP id q1so18115802wrp.0 for ; Tue, 02 Apr 2019 12:22:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cumulusnetworks.com; s=google; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-language:content-transfer-encoding; bh=WqXOKj/bIdCfdWmXHaFmzEA2l4UnmJXigykBt2l75Eg=; b=dW7jOUDCJy7g3PEJM3VV0qCbrw0MMlEDimSg+iruzNCOatt7Bg1HGxQE6fFJzFnfR4 9j4f4h21gRI2KUF4xLpYMPYt3BTxQYcvruPI7UzhXIQImtmREwF3o+IqxDUKWGXGIzh2 YyckTk/43gdu2UpAebcnlV7rW43j3TWK927/4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=WqXOKj/bIdCfdWmXHaFmzEA2l4UnmJXigykBt2l75Eg=; b=nWK68/uoJRkPz/gznGW025/oiRojNxav1skT7Z742ncMa+vYser9c0kMxF5ckN5YBR 41oqXQP09tzG2pSsqocPMvCPPzHT237SwbkxcMdKZd2vqTZvXPVEdQ2WpEsOy6MpYEIq w7tT+dyxzSmgrM+q94wQSyIf1cn2cXJHRVBbdnEp9ixcuqyc3V2NZBKkoJUQVqJ1wrHc tDRPrKLBYvmzpnNClsTfSC8ArhElQ0QePR6eY5V2IHmHczq9pwcx6pCpzimHWu+fR8te kE8ecvRKY97x835rf+asRCzusFKIiHNapq2B12nMRYMb0gHMEwyFvEv7H3Ol0shSw3EZ AHmw== X-Gm-Message-State: APjAAAXCXkEjtzqXVQ4SK1iGP/dKNAeSr61UYfVrl5nrGX2mQ4p0dd/e 4O7vOduFTlLjuF+9pk032lcRL4KUYOEGjA== X-Google-Smtp-Source: APXvYqxGff/qEG424raruCX1OD9Na7sYoFRPGMAC6KAlSKtHlOQL6iVAoLW/t/vNze/0ShUgXd5xYQ== X-Received: by 2002:a5d:538d:: with SMTP id d13mr3178096wrv.45.1554232932245; Tue, 02 Apr 2019 12:22:12 -0700 (PDT) Received: from [192.168.0.107] (84-238-136-197.ip.btc-net.bg. [84.238.136.197]) by smtp.gmail.com with ESMTPSA id e9sm22660327wrp.35.2019.04.02.12.22.11 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Tue, 02 Apr 2019 12:22:11 -0700 (PDT) Subject: Re: [PATCH net-next 3/4] bridge: support binding vlan dev link state to vlan member bridge ports To: Mike Manning , netdev@vger.kernel.org References: <20190402153543.6277-1-mmanning@vyatta.att-mail.com> <20190402153543.6277-4-mmanning@vyatta.att-mail.com> From: Nikolay Aleksandrov Message-ID: Date: Tue, 2 Apr 2019 22:22:10 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <20190402153543.6277-4-mmanning@vyatta.att-mail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > --- > 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; > + } > +} >