diff mbox

[net-next,4/4] bridge: vlan: combine (br|nbp)_vlan_flush into one

Message ID 1444650069-32572-5-git-send-email-razor@blackwall.org
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Nikolay Aleksandrov Oct. 12, 2015, 11:41 a.m. UTC 
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
unnecessary (and is actually a remnant of the old vlan code) so we can
remove it and combine both br/nbp vlan_flush functions into one.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_if.c      |  8 +++++---
 net/bridge/br_private.h |  9 ++-------
 net/bridge/br_vlan.c    | 16 +---------------
 3 files changed, 8 insertions(+), 25 deletions(-)

Comments

Ido Schimmel Oct. 12, 2015, 5:51 p.m. UTC | #1 
Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
>From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
>As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
>unnecessary (and is actually a remnant of the old vlan code) so we can
>remove it and combine both br/nbp vlan_flush functions into one.
Just a small note to Scott and Vivien:

One of the side effects of Nik's recent patchsets is that when VLANs are
flushed on a port the deletion is propagated to the driver via
switchdev ops, as __vlan_vid_del is called.

Therefore there is no need to do internal bookkeeping and remove VLANs
yourself when port is removed from bridge.
>
>Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivien Didelot Oct. 12, 2015, 6:15 p.m. UTC | #2 
Hi,

On Oct. Monday 12 (42) 08:51 PM, Ido Schimmel wrote:
> Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
> >From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> >
> >As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
> >unnecessary (and is actually a remnant of the old vlan code) so we can
> >remove it and combine both br/nbp vlan_flush functions into one.
> Just a small note to Scott and Vivien:
> 
> One of the side effects of Nik's recent patchsets is that when VLANs are
> flushed on a port the deletion is propagated to the driver via
> switchdev ops, as __vlan_vid_del is called.
> 
> Therefore there is no need to do internal bookkeeping and remove VLANs
> yourself when port is removed from bridge.

I was thinking about caching VLAN entries in the mv88e6xxx driver to
improve look up on VLAN and FDB operations, but it's a bit prematurate.

But when VLAN are flushed, we still need to remove them from the
hardware table, right?

Flushing is interesting though, most hardware have flush operations and
it would be interesting to have switchdev fdb_flush and vlan_flush ops.

Thanks!
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ido Schimmel Oct. 12, 2015, 6:27 p.m. UTC | #3 
Mon, Oct 12, 2015 at 09:15:39PM IDT, vivien.didelot@savoirfairelinux.com wrote:
>Hi,
>
>On Oct. Monday 12 (42) 08:51 PM, Ido Schimmel wrote:
>> Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
>> >From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> >
>> >As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
>> >unnecessary (and is actually a remnant of the old vlan code) so we can
>> >remove it and combine both br/nbp vlan_flush functions into one.
>> Just a small note to Scott and Vivien:
>> 
>> One of the side effects of Nik's recent patchsets is that when VLANs are
>> flushed on a port the deletion is propagated to the driver via
>> switchdev ops, as __vlan_vid_del is called.
>> 
>> Therefore there is no need to do internal bookkeeping and remove VLANs
>> yourself when port is removed from bridge.
>
>I was thinking about caching VLAN entries in the mv88e6xxx driver to
>improve look up on VLAN and FDB operations, but it's a bit prematurate.
>
>But when VLAN are flushed, we still need to remove them from the
>hardware table, right?
Hi,

Not sure I'm following. You'll simply get a SWITCHDEV_OBJ_ID_PORT_VLAN
(del) for each VLAN configured on the port you just removed from the bridge.
I guess you remove them from your hardware table in the implementation
of these ops?
>
>Flushing is interesting though, most hardware have flush operations and
>it would be interesting to have switchdev fdb_flush and vlan_flush ops.
>
>Thanks!
>-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivien Didelot Oct. 12, 2015, 7:49 p.m. UTC | #4 
On Oct. Monday 12 (42) 09:27 PM, Ido Schimmel wrote:
> Mon, Oct 12, 2015 at 09:15:39PM IDT, vivien.didelot@savoirfairelinux.com wrote:
> >Hi,
> >
> >On Oct. Monday 12 (42) 08:51 PM, Ido Schimmel wrote:
> >> Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
> >> >From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> >> >
> >> >As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
> >> >unnecessary (and is actually a remnant of the old vlan code) so we can
> >> >remove it and combine both br/nbp vlan_flush functions into one.
> >> Just a small note to Scott and Vivien:
> >> 
> >> One of the side effects of Nik's recent patchsets is that when VLANs are
> >> flushed on a port the deletion is propagated to the driver via
> >> switchdev ops, as __vlan_vid_del is called.
> >> 
> >> Therefore there is no need to do internal bookkeeping and remove VLANs
> >> yourself when port is removed from bridge.
> >
> >I was thinking about caching VLAN entries in the mv88e6xxx driver to
> >improve look up on VLAN and FDB operations, but it's a bit prematurate.
> >
> >But when VLAN are flushed, we still need to remove them from the
> >hardware table, right?
> Hi,
> 
> Not sure I'm following. You'll simply get a SWITCHDEV_OBJ_ID_PORT_VLAN
> (del) for each VLAN configured on the port you just removed from the bridge.
> I guess you remove them from your hardware table in the implementation
> of these ops?

Yes we do remove VLAN entries from the hardware table when
switchdev_port_obj_del(SWITCHDEV_OBJ_ID_PORT_VLAN) is called.

I may have misunderstood your previous note, I thought we were talking
about implementing the flush operation in switchdev drivers.

> >
> >Flushing is interesting though, most hardware have flush operations and
> >it would be interesting to have switchdev fdb_flush and vlan_flush ops.

Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Feldman Oct. 13, 2015, 6:41 a.m. UTC | #5 
On Mon, Oct 12, 2015 at 10:51 AM, Ido Schimmel <idosch@mellanox.com> wrote:
> Mon, Oct 12, 2015 at 02:41:09PM IDT, razor@blackwall.org wrote:
>>From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>
>>As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
>>unnecessary (and is actually a remnant of the old vlan code) so we can
>>remove it and combine both br/nbp vlan_flush functions into one.
> Just a small note to Scott and Vivien:
>
> One of the side effects of Nik's recent patchsets is that when VLANs are
> flushed on a port the deletion is propagated to the driver via
> switchdev ops, as __vlan_vid_del is called.
>
> Therefore there is no need to do internal bookkeeping and remove VLANs
> yourself when port is removed from bridge.

Thanks for the heads-up.  Seems like a nice result.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 74a03c0a4e5f..ed431cc80b3d 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -233,6 +233,7 @@  static void destroy_nbp_rcu(struct rcu_head *head)
  */
 static void del_nbp(struct net_bridge_port *p)
 {
+	struct net_bridge_vlan_group *vg;
 	struct net_bridge *br = p->br;
 	struct net_device *dev = p->dev;
 
@@ -249,7 +250,8 @@  static void del_nbp(struct net_bridge_port *p)
 	list_del_rcu(&p->list);
 
 	/* vlan_flush phase I: remove vlans */
-	nbp_vlan_flush(p, false);
+	vg = nbp_vlan_group(p);
+	br_vlan_flush(vg, false);
 	br_fdb_delete_by_port(br, p, 0, 1);
 	nbp_update_port_count(br);
 
@@ -261,7 +263,7 @@  static void del_nbp(struct net_bridge_port *p)
 	/* use the synchronize_rcu done by netdev_rx_handler_unregister
 	 * vlan_flush phase II: free rht and vlgrp
 	 */
-	nbp_vlan_flush(p, true);
+	br_vlan_flush(vg, true);
 
 	br_multicast_del_port(p);
 
@@ -286,7 +288,7 @@  void br_dev_delete(struct net_device *dev, struct list_head *head)
 	br_fdb_delete_by_port(br, NULL, 0, 1);
 
 	/* vlan_flush execute both phases (see del_nbp) */
-	br_vlan_flush(br, true);
+	br_vlan_flush(br_vlan_group(br), true);
 	br_multicast_dev_del(br);
 	del_timer_sync(&br->gc_timer);
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 3938a976417f..73ee71c0a960 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -682,7 +682,7 @@  struct sk_buff *br_handle_vlan(struct net_bridge *br,
 			       struct sk_buff *skb);
 int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
 int br_vlan_delete(struct net_bridge *br, u16 vid);
-void br_vlan_flush(struct net_bridge *br, bool free_rht);
+void br_vlan_flush(struct net_bridge_vlan_group *vg, bool free_rht);
 struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid);
 void br_recalculate_fwd_mask(struct net_bridge *br);
 int __br_vlan_filter_toggle(struct net_bridge *br, unsigned long val);
@@ -694,7 +694,6 @@  int br_vlan_set_default_pvid(struct net_bridge *br, unsigned long val);
 int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid);
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
 int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
-void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht);
 int nbp_vlan_init(struct net_bridge_port *port);
 int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
 
@@ -790,7 +789,7 @@  static inline int br_vlan_delete(struct net_bridge *br, u16 vid)
 	return -EOPNOTSUPP;
 }
 
-static inline void br_vlan_flush(struct net_bridge *br, bool free_rht)
+static inline void br_vlan_flush(struct net_bridge_vlan_group *vg, bool free_rht)
 {
 }
 
@@ -813,10 +812,6 @@  static inline int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	return -EOPNOTSUPP;
 }
 
-static inline void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht)
-{
-}
-
 static inline struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg,
 						   u16 vid)
 {
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 4fb9b23c9838..11ac14f60206 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -572,13 +572,6 @@  int br_vlan_delete(struct net_bridge *br, u16 vid)
 	return __vlan_del(v);
 }
 
-void br_vlan_flush(struct net_bridge *br, bool free_rht)
-{
-	ASSERT_RTNL();
-
-	__vlan_flush(br_vlan_group(br), free_rht);
-}
-
 struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid)
 {
 	if (!vg)
@@ -960,16 +953,9 @@  int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
 	return __vlan_del(v);
 }
 
-void nbp_vlan_flush(struct net_bridge_port *port, bool free_rht)
+void br_vlan_flush(struct net_bridge_vlan_group *vg, bool free_rht)
 {
-	struct net_bridge_vlan_group *vg;
-	struct net_bridge_vlan *vlan;
-
 	ASSERT_RTNL();
 
-	vg = nbp_vlan_group(port);
-	list_for_each_entry(vlan, &vg->vlan_list, vlist)
-		vlan_vid_del(port->dev, port->br->vlan_proto, vlan->vid);
-
 	__vlan_flush(vg, free_rht);
 }