From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <530E0B45.1090707@redhat.com> Date: Wed, 26 Feb 2014 10:41:57 -0500 From: Vlad Yasevich MIME-Version: 1.0 References: <1393427905-6811-1-git-send-email-vyasevic@redhat.com> <1393427905-6811-3-git-send-email-vyasevic@redhat.com> <20140226154122.GC15330@redhat.com> In-Reply-To: <20140226154122.GC15330@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Bridge] [PATCH 2/7] bridge: Keep track of ports capable of flooding. Reply-To: vyasevic@redhat.com List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: john.r.fastabend@intel.com, netdev@vger.kernel.org, shemminger@vyatta.com, bridge@lists.linux-foundation.org, jhs@mojatatu.com On 02/26/2014 10:41 AM, Michael S. Tsirkin wrote: > On Wed, Feb 26, 2014 at 10:18:20AM -0500, Vlad Yasevich wrote: >> Keep track of bridge ports that have unicast flooding turned on. >> This will later be used by the algorithm to automatically manage >> address programming and promisc mode. >> >> Signed-off-by: Vlad Yasevich >> --- >> net/bridge/br_if.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >> net/bridge/br_netlink.c | 3 +++ >> net/bridge/br_private.h | 4 ++++ >> net/bridge/br_sysfs_if.c | 6 ++++- >> 4 files changed, 70 insertions(+), 3 deletions(-) >> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index 54d207d..f072b34 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -27,6 +27,9 @@ >> >> #include "br_private.h" >> >> +static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br); >> +static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br); >> + > > same nitpick about forward declarations of static > functions. > maybe they are ok for bridge, but that's my $.02 > Ok. Will take a look. Just means that I might need to re-order the code which may scatter it a bit through the file. > >> /* >> * Determine initial path cost based on speed. >> * using recommendations from 802.1d standard >> @@ -141,11 +144,14 @@ static void del_nbp(struct net_bridge_port *p) >> >> br_ifinfo_notify(RTM_DELLINK, p); >> >> + list_del_rcu(&p->list); >> + >> + if (p->flags & BR_FLOOD) >> + br_del_flood_port(p, br); >> + >> nbp_vlan_flush(p); >> br_fdb_delete_by_port(br, p, 1); >> >> - list_del_rcu(&p->list); >> - >> dev->priv_flags &= ~IFF_BRIDGE_PORT; >> >> netdev_rx_handler_unregister(dev); > > ok so it doesn't matter in which order we do > list_del_rcu and nbp_vlan_flush then? Nope. vlan_flush knows which port its working and doesn't touch the port list. What does touch the port list if br_fdb_delete_by_port, but that only touches it in fdb_delete_local(), and it's ok there since we are trying to point an existing fdb out a different port. Thanks -vlad > >> @@ -383,6 +389,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) >> dev_disable_lro(dev); >> >> list_add_rcu(&p->list, &br->port_list); >> + >> + if (p->flags & BR_FLOOD) >> + br_add_flood_port(p, br); >> >> netdev_update_features(br->dev); >> >> @@ -455,3 +464,50 @@ int br_del_if(struct net_bridge *br, struct net_device *dev) >> >> return 0; >> } >> + >> +static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br) >> +{ >> + /* Increment the number of flooding ports, and if we >> + * only have 1 flooding port cache if for future use. >> + */ >> + br->n_flood_ports++; >> + if (br->n_flood_ports == 1) >> + br->c_flood_port = p; >> +} >> + >> +static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br) >> +{ >> + struct net_bridge_port *port; >> + >> + /* Decrement the number of flood port. >> + * If we are deleting the current flood port, clear >> + * the cached port. If we are down to 1 flood port, >> + * set it if it is not set. >> + */ >> + br->n_flood_ports--; >> + if (p == br->c_flood_port) >> + br->c_flood_port = NULL; >> + >> + if (br->n_flood_ports == 1) { >> + list_for_each_entry(port, &p->br->port_list, list) { >> + if (port->flags & BR_FLOOD) { >> + br->c_flood_port = port; >> + break; >> + } >> + } >> + } >> +} >> + >> +void br_port_flags_change(struct net_bridge_port *p, unsigned long mask) >> +{ >> + struct net_bridge *br = p->br; >> + >> + /* We are only interested FLOOD flag */ >> + if (!(mask & BR_FLOOD)) >> + return; >> + >> + if (p->flags & BR_FLOOD) >> + br_add_flood_port(p, br); >> + else >> + br_del_flood_port(p, br); >> +} >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >> index e74b6d53..01382b9 100644 >> --- a/net/bridge/br_netlink.c >> +++ b/net/bridge/br_netlink.c >> @@ -328,6 +328,7 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[], >> static int br_setport(struct net_bridge_port *p, struct nlattr *tb[]) >> { >> int err; >> + unsigned long old_flags = p->flags; >> >> br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE); >> br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD); >> @@ -353,6 +354,8 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[]) >> if (err) >> return err; >> } >> + >> + br_port_flags_change(p, old_flags ^ p->flags); >> return 0; >> } >> >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >> index 3ba11bc..26a3987 100644 >> --- a/net/bridge/br_private.h >> +++ b/net/bridge/br_private.h >> @@ -290,6 +290,8 @@ struct net_bridge >> struct timer_list topology_change_timer; >> struct timer_list gc_timer; >> struct kobject *ifobj; >> + struct net_bridge_port *c_flood_port; >> + u32 n_flood_ports; > > any hints on locking for these fields? > are these all under rtnl? > >> #ifdef CONFIG_BRIDGE_VLAN_FILTERING >> u8 vlan_enabled; >> struct net_port_vlans __rcu *vlan_info; >> @@ -415,6 +417,8 @@ int br_del_if(struct net_bridge *br, struct net_device *dev); >> int br_min_mtu(const struct net_bridge *br); >> netdev_features_t br_features_recompute(struct net_bridge *br, >> netdev_features_t features); >> +void br_port_flags_change(struct net_bridge_port *port, >> + unsigned long mask); >> >> /* br_input.c */ >> int br_handle_frame_finish(struct sk_buff *skb); >> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c >> index 7f66aa4..9ff6691 100644 >> --- a/net/bridge/br_sysfs_if.c >> +++ b/net/bridge/br_sysfs_if.c >> @@ -51,7 +51,10 @@ static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR, \ >> static int store_flag(struct net_bridge_port *p, unsigned long v, >> unsigned long mask) >> { >> - unsigned long flags = p->flags; >> + unsigned long flags; >> + unsigned long old_flags; >> + >> + old_flags = flags = p->flags; >> >> if (v) >> flags |= mask; >> @@ -60,6 +63,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v, >> >> if (flags != p->flags) { >> p->flags = flags; >> + br_port_flags_change(p, old_flags ^ flags); >> br_ifinfo_notify(RTM_NEWLINK, p); >> } >> return 0; >> -- >> 1.8.5.3 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Yasevich Subject: Re: [PATCH 2/7] bridge: Keep track of ports capable of flooding. Date: Wed, 26 Feb 2014 10:41:57 -0500 Message-ID: <530E0B45.1090707@redhat.com> References: <1393427905-6811-1-git-send-email-vyasevic@redhat.com> <1393427905-6811-3-git-send-email-vyasevic@redhat.com> <20140226154122.GC15330@redhat.com> Reply-To: vyasevic@redhat.com Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, bridge@lists.linux-foundation.org, shemminger@vyatta.com, jhs@mojatatu.com, john.r.fastabend@intel.com To: "Michael S. Tsirkin" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:33994 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751157AbaBZQWp (ORCPT ); Wed, 26 Feb 2014 11:22:45 -0500 In-Reply-To: <20140226154122.GC15330@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On 02/26/2014 10:41 AM, Michael S. Tsirkin wrote: > On Wed, Feb 26, 2014 at 10:18:20AM -0500, Vlad Yasevich wrote: >> Keep track of bridge ports that have unicast flooding turned on. >> This will later be used by the algorithm to automatically manage >> address programming and promisc mode. >> >> Signed-off-by: Vlad Yasevich >> --- >> net/bridge/br_if.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- >> net/bridge/br_netlink.c | 3 +++ >> net/bridge/br_private.h | 4 ++++ >> net/bridge/br_sysfs_if.c | 6 ++++- >> 4 files changed, 70 insertions(+), 3 deletions(-) >> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index 54d207d..f072b34 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -27,6 +27,9 @@ >> >> #include "br_private.h" >> >> +static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br); >> +static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br); >> + > > same nitpick about forward declarations of static > functions. > maybe they are ok for bridge, but that's my $.02 > Ok. Will take a look. Just means that I might need to re-order the code which may scatter it a bit through the file. > >> /* >> * Determine initial path cost based on speed. >> * using recommendations from 802.1d standard >> @@ -141,11 +144,14 @@ static void del_nbp(struct net_bridge_port *p) >> >> br_ifinfo_notify(RTM_DELLINK, p); >> >> + list_del_rcu(&p->list); >> + >> + if (p->flags & BR_FLOOD) >> + br_del_flood_port(p, br); >> + >> nbp_vlan_flush(p); >> br_fdb_delete_by_port(br, p, 1); >> >> - list_del_rcu(&p->list); >> - >> dev->priv_flags &= ~IFF_BRIDGE_PORT; >> >> netdev_rx_handler_unregister(dev); > > ok so it doesn't matter in which order we do > list_del_rcu and nbp_vlan_flush then? Nope. vlan_flush knows which port its working and doesn't touch the port list. What does touch the port list if br_fdb_delete_by_port, but that only touches it in fdb_delete_local(), and it's ok there since we are trying to point an existing fdb out a different port. Thanks -vlad > >> @@ -383,6 +389,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev) >> dev_disable_lro(dev); >> >> list_add_rcu(&p->list, &br->port_list); >> + >> + if (p->flags & BR_FLOOD) >> + br_add_flood_port(p, br); >> >> netdev_update_features(br->dev); >> >> @@ -455,3 +464,50 @@ int br_del_if(struct net_bridge *br, struct net_device *dev) >> >> return 0; >> } >> + >> +static void br_add_flood_port(struct net_bridge_port *p, struct net_bridge *br) >> +{ >> + /* Increment the number of flooding ports, and if we >> + * only have 1 flooding port cache if for future use. >> + */ >> + br->n_flood_ports++; >> + if (br->n_flood_ports == 1) >> + br->c_flood_port = p; >> +} >> + >> +static void br_del_flood_port(struct net_bridge_port *p, struct net_bridge *br) >> +{ >> + struct net_bridge_port *port; >> + >> + /* Decrement the number of flood port. >> + * If we are deleting the current flood port, clear >> + * the cached port. If we are down to 1 flood port, >> + * set it if it is not set. >> + */ >> + br->n_flood_ports--; >> + if (p == br->c_flood_port) >> + br->c_flood_port = NULL; >> + >> + if (br->n_flood_ports == 1) { >> + list_for_each_entry(port, &p->br->port_list, list) { >> + if (port->flags & BR_FLOOD) { >> + br->c_flood_port = port; >> + break; >> + } >> + } >> + } >> +} >> + >> +void br_port_flags_change(struct net_bridge_port *p, unsigned long mask) >> +{ >> + struct net_bridge *br = p->br; >> + >> + /* We are only interested FLOOD flag */ >> + if (!(mask & BR_FLOOD)) >> + return; >> + >> + if (p->flags & BR_FLOOD) >> + br_add_flood_port(p, br); >> + else >> + br_del_flood_port(p, br); >> +} >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >> index e74b6d53..01382b9 100644 >> --- a/net/bridge/br_netlink.c >> +++ b/net/bridge/br_netlink.c >> @@ -328,6 +328,7 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[], >> static int br_setport(struct net_bridge_port *p, struct nlattr *tb[]) >> { >> int err; >> + unsigned long old_flags = p->flags; >> >> br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE); >> br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD); >> @@ -353,6 +354,8 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[]) >> if (err) >> return err; >> } >> + >> + br_port_flags_change(p, old_flags ^ p->flags); >> return 0; >> } >> >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >> index 3ba11bc..26a3987 100644 >> --- a/net/bridge/br_private.h >> +++ b/net/bridge/br_private.h >> @@ -290,6 +290,8 @@ struct net_bridge >> struct timer_list topology_change_timer; >> struct timer_list gc_timer; >> struct kobject *ifobj; >> + struct net_bridge_port *c_flood_port; >> + u32 n_flood_ports; > > any hints on locking for these fields? > are these all under rtnl? > >> #ifdef CONFIG_BRIDGE_VLAN_FILTERING >> u8 vlan_enabled; >> struct net_port_vlans __rcu *vlan_info; >> @@ -415,6 +417,8 @@ int br_del_if(struct net_bridge *br, struct net_device *dev); >> int br_min_mtu(const struct net_bridge *br); >> netdev_features_t br_features_recompute(struct net_bridge *br, >> netdev_features_t features); >> +void br_port_flags_change(struct net_bridge_port *port, >> + unsigned long mask); >> >> /* br_input.c */ >> int br_handle_frame_finish(struct sk_buff *skb); >> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c >> index 7f66aa4..9ff6691 100644 >> --- a/net/bridge/br_sysfs_if.c >> +++ b/net/bridge/br_sysfs_if.c >> @@ -51,7 +51,10 @@ static BRPORT_ATTR(_name, S_IRUGO | S_IWUSR, \ >> static int store_flag(struct net_bridge_port *p, unsigned long v, >> unsigned long mask) >> { >> - unsigned long flags = p->flags; >> + unsigned long flags; >> + unsigned long old_flags; >> + >> + old_flags = flags = p->flags; >> >> if (v) >> flags |= mask; >> @@ -60,6 +63,7 @@ static int store_flag(struct net_bridge_port *p, unsigned long v, >> >> if (flags != p->flags) { >> p->flags = flags; >> + br_port_flags_change(p, old_flags ^ flags); >> br_ifinfo_notify(RTM_NEWLINK, p); >> } >> return 0; >> -- >> 1.8.5.3