[PATCH net-next v2 0/6] bonding: remove bond->vlan_list

[PATCH net-next v2 0/6] bonding: remove bond->vlan_list

[Veaceslav Falico]

RFC -> v1: Got some feedback from Nikolay Aleksandrov (privately), tried to
	   address it, also fixed some bugs that I've found on the way. I
	   think it's ready to be considered a patchset for
	   review/inclusion in net-next.

v1  -> v2: Remove ASSERT_RTNL() from vlan_uses_dev(), cause it can be
	   already called under rcu, without rtnl. Don't check for master
	   device in __vlan_find_dev_next(), otherwise we won't be able to
	   work in situations when a device has both vlans and master
	   device. Properly init vlan_dev in bond_has_this_ip() before
	   using (sigh). There was a proposal of making a macro
	   "dev_for_each_vlan_from(dev, vlan_dev, i, from)", which would
	   use __vlan_find_dev_deep() inside, with its strong and weak
	   parts, but I've decided to stick to the "while (dev = next())"
	   scheme currently - it might be added anytime, and now the only
	   user (bonding) doesn't really need it.

The aim of this patchset is to remove bond->vlan_list completely, and use
8021q's standard functions instead of it.

The patchset is on top of Nik's latest two patches:
[net-next,v2,1/2] bonding: change the bond's vlan syncing functions with
			   the standard ones
[net-next,v2,2/2] bonding: unwind on bond_add_vlan failure


First two patches add two helper functions to vlan code:

bonding: add rcu to vlan_uses_dev() and make bond_vlan_used() use it

	Here we change vlan_uses_dev() to be able to work under both rtnl
	and rcu, and use it under rcu_read_lock() in bond_vlan_used().

vlan: add __vlan_find_dev_next()

	This function takes dev and vlan_dev and returns the next vlan dev
	that uses this dev. It can be used to cycle through the vlan list,
	and not only by bonding - but by any network driver that uses its
	private vlan list.

Next four patches actually convert bonding to use the new
functions/approach and remove the vlan_list completely.

This patchset solves several issues with bonding, simplify it overall,
RCUify further and add infrastructure to anyone else who'd like to use
8021q standard functions instead of their own vlan_list, which is quite
common amongst network drivers currently.

I'm testing it continuously currently, no issues found, will update on
anything.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Patrick McHardy <kaber@trash.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

---
 drivers/net/bonding/bond_alb.c  |   48 ++++++++----
 drivers/net/bonding/bond_alb.h  |    2 +-
 drivers/net/bonding/bond_main.c |  163 ++++++---------------------------------
 drivers/net/bonding/bonding.h   |   16 ++--
 include/linux/if_vlan.h         |    8 ++
 net/8021q/vlan.h                |    6 +-
 net/8021q/vlan_core.c           |   36 ++++++++-
 7 files changed, 115 insertions(+), 164 deletions(-)

[PATCH v2 net-next 1/6] bonding: add rcu to vlan_uses_dev() and make bond_vlan_used() use it

[Veaceslav Falico]

RFC -> v1: don't add vlan_uses_dev_rcu() and change vlan_uses_dev() instead
v1  -> v2: remove the ASSERT_RTNL(), cause we can now be called under rcu.

Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
id 0, and always returns true if 8021q is loaded. This creates several bad
situations - some warnings in __bond_release_one() because it thinks that
we still have vlans while removing, sending LB packets with vlan id 0 and,
possibly, other caused by vlan id 0.

Fix it by changing vlan_uses_dev() to use rcu_dereference_rtnl() instead of
rtnl_dereference(), and thus it can already be used in bond_vlan_used()
under rcu_read_lock().

By the time vlan_uses_dev() returns we cannot be sure if there were no
vlans added/removed, so it's basicly an optimization function.

Also, use the vlan_uses_dev() in __bond_release_one() cause the rtnl lock
is held there.

For this call to be visible in bonding.h, add include <linux/if_vlan.h>,
and also remove it from any other bonding file, cause they all include
bonding.h, and thus linux/if_vlan.h.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Patrick McHardy <kaber@trash.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Nikolay Aleksandrov <nikolay@redhat.com>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_alb.c  |    1 -
 drivers/net/bonding/bond_main.c |    3 +--
 drivers/net/bonding/bonding.h   |   10 +++++++++-
 net/8021q/vlan_core.c           |    5 ++---
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 3a5db7b..2684329 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -34,7 +34,6 @@
 #include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <linux/if_bonding.h>
-#include <linux/if_vlan.h>
 #include <linux/in.h>
 #include <net/ipx.h>
 #include <net/arp.h>
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4264a76..9d1045d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -69,7 +69,6 @@
 #include <net/arp.h>
 #include <linux/mii.h>
 #include <linux/ethtool.h>
-#include <linux/if_vlan.h>
 #include <linux/if_bonding.h>
 #include <linux/jiffies.h>
 #include <linux/preempt.h>
@@ -1953,7 +1952,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 		bond_set_carrier(bond);
 		eth_hw_addr_random(bond_dev);
 
-		if (bond_vlan_used(bond)) {
+		if (vlan_uses_dev(bond_dev)) {
 			pr_warning("%s: Warning: clearing HW address of %s while it still has VLANs.\n",
 				   bond_dev->name, bond_dev->name);
 			pr_warning("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs'.\n",
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 4bf52d5..9c4539e 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -23,6 +23,7 @@
 #include <linux/netpoll.h>
 #include <linux/inetdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include "bond_3ad.h"
 #include "bond_alb.h"
 
@@ -267,9 +268,16 @@ struct bonding {
 #endif /* CONFIG_DEBUG_FS */
 };
 
+/* use vlan_uses_dev() if under rtnl */
 static inline bool bond_vlan_used(struct bonding *bond)
 {
-	return !list_empty(&bond->vlan_list);
+	bool ret;
+
+	rcu_read_lock();
+	ret = vlan_uses_dev(bond->dev);
+	rcu_read_unlock();
+
+	return ret;
 }
 
 #define bond_slave_get_rcu(dev) \
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 4a78c4d..361c97b 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -387,13 +387,12 @@ void vlan_vids_del_by_dev(struct net_device *dev,
 }
 EXPORT_SYMBOL(vlan_vids_del_by_dev);
 
+/* Must hold either rtnl or rcu_read_lock */
 bool vlan_uses_dev(const struct net_device *dev)
 {
 	struct vlan_info *vlan_info;
 
-	ASSERT_RTNL();
-
-	vlan_info = rtnl_dereference(dev->vlan_info);
+	vlan_info = rcu_dereference_rtnl(dev->vlan_info);
 	if (!vlan_info)
 		return false;
 	return vlan_info->grp.nr_vlan_devs ? true : false;
-- 
1.7.1

Re: [PATCH v2 net-next 1/6] bonding: add rcu to vlan_uses_dev() and make bond_vlan_used() use it

[Nikolay Aleksandrov]

On 08/08/2013 06:57 PM, Veaceslav Falico wrote:
> RFC -> v1: don't add vlan_uses_dev_rcu() and change vlan_uses_dev() instead
> v1  -> v2: remove the ASSERT_RTNL(), cause we can now be called under rcu.
> 
> Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
> id 0, and always returns true if 8021q is loaded. This creates several bad
> situations - some warnings in __bond_release_one() because it thinks that
> we still have vlans while removing, sending LB packets with vlan id 0 and,
> possibly, other caused by vlan id 0.
> 
> Fix it by changing vlan_uses_dev() to use rcu_dereference_rtnl() instead of
> rtnl_dereference(), and thus it can already be used in bond_vlan_used()
> under rcu_read_lock().
> 
> By the time vlan_uses_dev() returns we cannot be sure if there were no
> vlans added/removed, so it's basicly an optimization function.
> 
> Also, use the vlan_uses_dev() in __bond_release_one() cause the rtnl lock
> is held there.
> 
> For this call to be visible in bonding.h, add include <linux/if_vlan.h>,
> and also remove it from any other bonding file, cause they all include
> bonding.h, and thus linux/if_vlan.h.
> 
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> CC: Patrick McHardy <kaber@trash.net>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Nikolay Aleksandrov <nikolay@redhat.com>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>  drivers/net/bonding/bond_alb.c  |    1 -
>  drivers/net/bonding/bond_main.c |    3 +--
>  drivers/net/bonding/bonding.h   |   10 +++++++++-
>  net/8021q/vlan_core.c           |    5 ++---
>  4 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 3a5db7b..2684329 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -34,7 +34,6 @@
>  #include <linux/if_arp.h>
>  #include <linux/if_ether.h>
>  #include <linux/if_bonding.h>
> -#include <linux/if_vlan.h>
>  #include <linux/in.h>
>  #include <net/ipx.h>
>  #include <net/arp.h>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 4264a76..9d1045d 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -69,7 +69,6 @@
>  #include <net/arp.h>
>  #include <linux/mii.h>
>  #include <linux/ethtool.h>
> -#include <linux/if_vlan.h>
>  #include <linux/if_bonding.h>
>  #include <linux/jiffies.h>
>  #include <linux/preempt.h>
> @@ -1953,7 +1952,7 @@ static int __bond_release_one(struct net_device *bond_dev,
>  		bond_set_carrier(bond);
>  		eth_hw_addr_random(bond_dev);
>  
> -		if (bond_vlan_used(bond)) {
> +		if (vlan_uses_dev(bond_dev)) {
>  			pr_warning("%s: Warning: clearing HW address of %s while it still has VLANs.\n",
>  				   bond_dev->name, bond_dev->name);
>  			pr_warning("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs'.\n",
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index 4bf52d5..9c4539e 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -23,6 +23,7 @@
>  #include <linux/netpoll.h>
>  #include <linux/inetdevice.h>
>  #include <linux/etherdevice.h>
> +#include <linux/if_vlan.h>
>  #include "bond_3ad.h"
>  #include "bond_alb.h"
>  
> @@ -267,9 +268,16 @@ struct bonding {
>  #endif /* CONFIG_DEBUG_FS */
>  };
>  
> +/* use vlan_uses_dev() if under rtnl */
Small nitpick - I don't think this comment is necessary as both calls now reduce
to vlan_uses_dev() and can be used interchangeably, maybe it's better to stick
to bond_vlan_used() just for consistency everywhere.
Anyway, I'm okay with this version as well:

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

Re: [PATCH v2 net-next 1/6] bonding: add rcu to vlan_uses_dev() and make bond_vlan_used() use it

[Veaceslav Falico]

On Fri, Aug 09, 2013 at 01:06:51PM +0200, Nikolay Aleksandrov wrote:
>On 08/08/2013 06:57 PM, Veaceslav Falico wrote:
>> RFC -> v1: don't add vlan_uses_dev_rcu() and change vlan_uses_dev() instead
>> v1  -> v2: remove the ASSERT_RTNL(), cause we can now be called under rcu.
>>
>> Currently, bond_vlan_used() looks for any vlan, including the pseudo-vlan
>> id 0, and always returns true if 8021q is loaded. This creates several bad
>> situations - some warnings in __bond_release_one() because it thinks that
>> we still have vlans while removing, sending LB packets with vlan id 0 and,
>> possibly, other caused by vlan id 0.
>>
>> Fix it by changing vlan_uses_dev() to use rcu_dereference_rtnl() instead of
>> rtnl_dereference(), and thus it can already be used in bond_vlan_used()
>> under rcu_read_lock().
>>
>> By the time vlan_uses_dev() returns we cannot be sure if there were no
>> vlans added/removed, so it's basicly an optimization function.
>>
>> Also, use the vlan_uses_dev() in __bond_release_one() cause the rtnl lock
>> is held there.
>>
>> For this call to be visible in bonding.h, add include <linux/if_vlan.h>,
>> and also remove it from any other bonding file, cause they all include
>> bonding.h, and thus linux/if_vlan.h.
>>
>> CC: Jay Vosburgh <fubar@us.ibm.com>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> CC: Patrick McHardy <kaber@trash.net>
>> CC: "David S. Miller" <davem@davemloft.net>
>> CC: Nikolay Aleksandrov <nikolay@redhat.com>
>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>> ---
>>  drivers/net/bonding/bond_alb.c  |    1 -
>>  drivers/net/bonding/bond_main.c |    3 +--
>>  drivers/net/bonding/bonding.h   |   10 +++++++++-
>>  net/8021q/vlan_core.c           |    5 ++---
>>  4 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index 3a5db7b..2684329 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -34,7 +34,6 @@
>>  #include <linux/if_arp.h>
>>  #include <linux/if_ether.h>
>>  #include <linux/if_bonding.h>
>> -#include <linux/if_vlan.h>
>>  #include <linux/in.h>
>>  #include <net/ipx.h>
>>  #include <net/arp.h>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 4264a76..9d1045d 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -69,7 +69,6 @@
>>  #include <net/arp.h>
>>  #include <linux/mii.h>
>>  #include <linux/ethtool.h>
>> -#include <linux/if_vlan.h>
>>  #include <linux/if_bonding.h>
>>  #include <linux/jiffies.h>
>>  #include <linux/preempt.h>
>> @@ -1953,7 +1952,7 @@ static int __bond_release_one(struct net_device *bond_dev,
>>  		bond_set_carrier(bond);
>>  		eth_hw_addr_random(bond_dev);
>>
>> -		if (bond_vlan_used(bond)) {
>> +		if (vlan_uses_dev(bond_dev)) {
>>  			pr_warning("%s: Warning: clearing HW address of %s while it still has VLANs.\n",
>>  				   bond_dev->name, bond_dev->name);
>>  			pr_warning("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs'.\n",
>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>> index 4bf52d5..9c4539e 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -23,6 +23,7 @@
>>  #include <linux/netpoll.h>
>>  #include <linux/inetdevice.h>
>>  #include <linux/etherdevice.h>
>> +#include <linux/if_vlan.h>
>>  #include "bond_3ad.h"
>>  #include "bond_alb.h"
>>
>> @@ -267,9 +268,16 @@ struct bonding {
>>  #endif /* CONFIG_DEBUG_FS */
>>  };
>>
>> +/* use vlan_uses_dev() if under rtnl */
>Small nitpick - I don't think this comment is necessary as both calls now reduce
>to vlan_uses_dev() and can be used interchangeably, maybe it's better to stick
>to bond_vlan_used() just for consistency everywhere.
>Anyway, I'm okay with this version as well:

Good catch, leftover from previous version.

>
>Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

Thank you.

>

[PATCH v2 net-next 2/6] vlan: add __vlan_find_dev_next()

[Veaceslav Falico]

RFC -> v1: make the function accept/return vlan's net_device, this way we
	   won't have troubles with VLAN 0, and the user code will be
	   cleaner and faster.
v1  -> v2: don't check for the master device - if we'll want in the future
	   to convert a slave device - it will fail, but now we don't
	   actually care.

Add a new exported function __vlan_find_dev_next(dev, vlan_dev), which
returns the a vlan's net_device that is used by the dev and is its id is
greater or equal to vlan_dev's vlan id. If vlan_dev is NULL, return first
vlan, if nothing is found return NULL.

This function must be under rcu_read_lock(), is aware of master devices and
doesn't guarantee that, once it returns, the vlan dev will still be used by
dev as its vlan.

It's basically a helper for "for_each_vlan_in_dev(dev, vlan_dev)" logic,
and is supposed to be used like this:

vlan_dev = NULL;

while ((vlan_dev = __vlan_find_dev_next(dev, vlan_dev))) {
	if (!vlan_dev)
		continue;

	do_work(vlan_dev);
}

In that case we're sure that vlan_dev at least was used as a vlan, and won't
go away while we're holding rcu_read_lock().

However, if we don't hold rtnl_lock(), we can't be sure that that vlan_dev
is still in dev's vlan_list.

CC: Patrick McHardy <kaber@trash.net>
CC: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 include/linux/if_vlan.h |    8 ++++++++
 net/8021q/vlan.h        |    6 ++++--
 net/8021q/vlan_core.c   |   28 ++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 715c343..1cfc201 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -86,6 +86,8 @@ static inline int is_vlan_dev(struct net_device *dev)
 
 extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
 					       __be16 vlan_proto, u16 vlan_id);
+extern struct net_device *__vlan_find_dev_next(struct net_device *dev,
+					       struct net_device *vlan_dev);
 extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
 extern u16 vlan_dev_vlan_id(const struct net_device *dev);
 
@@ -109,6 +111,12 @@ __vlan_find_dev_deep(struct net_device *real_dev,
 	return NULL;
 }
 
+static struct net_device *__vlan_find_dev_next(struct net_device *dev,
+					       struct net_device *vlan_dev)
+{
+	return NULL;
+}
+
 static inline struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
 	BUG();
diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
index ba5983f..e13aeac 100644
--- a/net/8021q/vlan.h
+++ b/net/8021q/vlan.h
@@ -168,10 +168,12 @@ static inline struct net_device *vlan_find_dev(struct net_device *real_dev,
 	return NULL;
 }
 
-#define vlan_group_for_each_dev(grp, i, dev) \
-	for ((i) = 0; i < VLAN_PROTO_NUM * VLAN_N_VID; i++) \
+#define vlan_group_for_each_dev_from(grp, i, dev, from) \
+	for ((i) = from; i < VLAN_PROTO_NUM * VLAN_N_VID; i++) \
 		if (((dev) = __vlan_group_get_device((grp), (i) / VLAN_N_VID, \
 							    (i) % VLAN_N_VID)))
+#define vlan_group_for_each_dev(grp, i, dev) \
+	vlan_group_for_each_dev_from(grp, i, dev, 0)
 
 /* found in vlan_dev.c */
 void vlan_dev_set_ingress_priority(const struct net_device *dev,
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 361c97b..c0031c3 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -89,6 +89,34 @@ struct net_device *__vlan_find_dev_deep(struct net_device *dev,
 }
 EXPORT_SYMBOL(__vlan_find_dev_deep);
 
+/* Must be called under rcu_read_lock(), returns next vlan_id used by a
+ * vlan device on dev, starting with vlan_id, or 0 if no vlan_id found.
+ */
+struct net_device *__vlan_find_dev_next(struct net_device *dev,
+					struct net_device *vlan_dev)
+{
+	struct net_device *ret;
+	struct vlan_info *vlan_info;
+	struct vlan_group *grp;
+	u16 vlan_id = 0, i;
+
+	if (vlan_dev)
+		vlan_id = vlan_dev_priv(vlan_dev)->vlan_id + 1;
+
+	vlan_info = rcu_dereference(dev->vlan_info);
+
+	if (!vlan_info)
+		return NULL;
+
+	grp = &vlan_info->grp;
+
+	vlan_group_for_each_dev_from(grp, i, ret, vlan_id)
+		return ret;
+
+	return NULL;
+}
+EXPORT_SYMBOL(__vlan_find_dev_next);
+
 struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
 	return vlan_dev_priv(dev)->real_dev;
-- 
1.7.1

Re: [PATCH v2 net-next 2/6] vlan: add __vlan_find_dev_next()

[Veaceslav Falico]

On Thu, Aug 08, 2013 at 06:57:55PM +0200, Veaceslav Falico wrote:
>RFC -> v1: make the function accept/return vlan's net_device, this way we
>	   won't have troubles with VLAN 0, and the user code will be
>	   cleaner and faster.
>v1  -> v2: don't check for the master device - if we'll want in the future
>	   to convert a slave device - it will fail, but now we don't
>	   actually care.
>
>Add a new exported function __vlan_find_dev_next(dev, vlan_dev), which
>returns the a vlan's net_device that is used by the dev and is its id is
>greater or equal to vlan_dev's vlan id. If vlan_dev is NULL, return first
>vlan, if nothing is found return NULL.
>
>This function must be under rcu_read_lock(), is aware of master devices and
>doesn't guarantee that, once it returns, the vlan dev will still be used by
>dev as its vlan.
>
>It's basically a helper for "for_each_vlan_in_dev(dev, vlan_dev)" logic,
>and is supposed to be used like this:
>
>vlan_dev = NULL;
>
>while ((vlan_dev = __vlan_find_dev_next(dev, vlan_dev))) {
>	if (!vlan_dev)
>		continue;
	^^^^^^^^^^^^^^^^^
Hm, that's completely useless, leftover from previous commit messages, will
remove.
>
>	do_work(vlan_dev);
>}
>
>In that case we're sure that vlan_dev at least was used as a vlan, and won't
>go away while we're holding rcu_read_lock().
>
>However, if we don't hold rtnl_lock(), we can't be sure that that vlan_dev
>is still in dev's vlan_list.
>
>CC: Patrick McHardy <kaber@trash.net>
>CC: "David S. Miller" <davem@davemloft.net>
>Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>---
> include/linux/if_vlan.h |    8 ++++++++
> net/8021q/vlan.h        |    6 ++++--
> net/8021q/vlan_core.c   |   28 ++++++++++++++++++++++++++++
> 3 files changed, 40 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>index 715c343..1cfc201 100644
>--- a/include/linux/if_vlan.h
>+++ b/include/linux/if_vlan.h
>@@ -86,6 +86,8 @@ static inline int is_vlan_dev(struct net_device *dev)
>
> extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
> 					       __be16 vlan_proto, u16 vlan_id);
>+extern struct net_device *__vlan_find_dev_next(struct net_device *dev,
>+					       struct net_device *vlan_dev);
> extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
> extern u16 vlan_dev_vlan_id(const struct net_device *dev);
>
>@@ -109,6 +111,12 @@ __vlan_find_dev_deep(struct net_device *real_dev,
> 	return NULL;
> }
>
>+static struct net_device *__vlan_find_dev_next(struct net_device *dev,
>+					       struct net_device *vlan_dev)
>+{
>+	return NULL;
>+}
>+
> static inline struct net_device *vlan_dev_real_dev(const struct net_device *dev)
> {
> 	BUG();
>diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
>index ba5983f..e13aeac 100644
>--- a/net/8021q/vlan.h
>+++ b/net/8021q/vlan.h
>@@ -168,10 +168,12 @@ static inline struct net_device *vlan_find_dev(struct net_device *real_dev,
> 	return NULL;
> }
>
>-#define vlan_group_for_each_dev(grp, i, dev) \
>-	for ((i) = 0; i < VLAN_PROTO_NUM * VLAN_N_VID; i++) \
>+#define vlan_group_for_each_dev_from(grp, i, dev, from) \
>+	for ((i) = from; i < VLAN_PROTO_NUM * VLAN_N_VID; i++) \
> 		if (((dev) = __vlan_group_get_device((grp), (i) / VLAN_N_VID, \
> 							    (i) % VLAN_N_VID)))
>+#define vlan_group_for_each_dev(grp, i, dev) \
>+	vlan_group_for_each_dev_from(grp, i, dev, 0)
>
> /* found in vlan_dev.c */
> void vlan_dev_set_ingress_priority(const struct net_device *dev,
>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>index 361c97b..c0031c3 100644
>--- a/net/8021q/vlan_core.c
>+++ b/net/8021q/vlan_core.c
>@@ -89,6 +89,34 @@ struct net_device *__vlan_find_dev_deep(struct net_device *dev,
> }
> EXPORT_SYMBOL(__vlan_find_dev_deep);
>
>+/* Must be called under rcu_read_lock(), returns next vlan_id used by a
>+ * vlan device on dev, starting with vlan_id, or 0 if no vlan_id found.
>+ */
>+struct net_device *__vlan_find_dev_next(struct net_device *dev,
>+					struct net_device *vlan_dev)
>+{
>+	struct net_device *ret;
>+	struct vlan_info *vlan_info;
>+	struct vlan_group *grp;
>+	u16 vlan_id = 0, i;
>+
>+	if (vlan_dev)
>+		vlan_id = vlan_dev_priv(vlan_dev)->vlan_id + 1;
>+
>+	vlan_info = rcu_dereference(dev->vlan_info);
>+
>+	if (!vlan_info)
>+		return NULL;
>+
>+	grp = &vlan_info->grp;
>+
>+	vlan_group_for_each_dev_from(grp, i, ret, vlan_id)
>+		return ret;
>+
>+	return NULL;
>+}
>+EXPORT_SYMBOL(__vlan_find_dev_next);
>+
> struct net_device *vlan_dev_real_dev(const struct net_device *dev)
> {
> 	return vlan_dev_priv(dev)->real_dev;
>-- 
>1.7.1
>

Re: [PATCH v2 net-next 2/6] vlan: add __vlan_find_dev_next()

[Nikolay Aleksandrov]

On 08/08/2013 06:57 PM, Veaceslav Falico wrote:
> RFC -> v1: make the function accept/return vlan's net_device, this way we
> 	   won't have troubles with VLAN 0, and the user code will be
> 	   cleaner and faster.
> v1  -> v2: don't check for the master device - if we'll want in the future
> 	   to convert a slave device - it will fail, but now we don't
> 	   actually care.
> 
> Add a new exported function __vlan_find_dev_next(dev, vlan_dev), which
> returns the a vlan's net_device that is used by the dev and is its id is
> greater or equal to vlan_dev's vlan id. If vlan_dev is NULL, return first
> vlan, if nothing is found return NULL.
> 
> This function must be under rcu_read_lock(), is aware of master devices and
> doesn't guarantee that, once it returns, the vlan dev will still be used by
> dev as its vlan.
> 
> It's basically a helper for "for_each_vlan_in_dev(dev, vlan_dev)" logic,
> and is supposed to be used like this:
> 
> vlan_dev = NULL;
> 
> while ((vlan_dev = __vlan_find_dev_next(dev, vlan_dev))) {
> 	if (!vlan_dev)
> 		continue;
> 
> 	do_work(vlan_dev);
> }
> 
> In that case we're sure that vlan_dev at least was used as a vlan, and won't
> go away while we're holding rcu_read_lock().
> 
> However, if we don't hold rtnl_lock(), we can't be sure that that vlan_dev
> is still in dev's vlan_list.
> 
> CC: Patrick McHardy <kaber@trash.net>
> CC: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---

We already discussed privately my comments about this new function, I'll leave
them here just for the sake of having them documented:
My proposition is to drop these changes altogether (the new function, and the
vlan.h macro change) and instead add something like the following (I haven't
tested it, it's only to illustrate the idea) in if_vlan.h so it'll be accessible
to everyone:

Version 1 (circular so you can simplify the bonding ALB code, I haven't left
spaces because of auto-wrapping):

#define dev_for_each_vlan_from(dev, vlandev, proto, from, i)
	for (i=from+1, vlandev=__vlan_find_dev_deep(dev, proto, from); \
	     i!=from; \
	     i=(i+1)%VLAN_N_VID, vlandev=__vlan_find_dev_deep(dev, proto, i)) \
		if (vlandev)

#define dev_for_each_vlan(dev, vlandev, proto, i)
	dev_for_each_vlan_from(dev, vlandev, proto, 0, i)

Version 2 is the same but a little shorter, without the circular part.
This way you reuse the already provided function __vlan_find_dev_deep and the
churn is smaller, also __vlan_find_dev_deep takes care of the master issue (that
is if dev doesn't have a vlan_info, then its master's vlan_info will be used).
Also the code will look much nicer IMO changing this:
while ((vlan_dev = __vlan_find_dev_next))

to

dev_for_each_vlan(dev, vlan_dev, proto, i)

There're also 2 nice side-effects, first you'll only walk over 4096 entries (for
the specified vlan proto only) and the bonding ALB code will simplify from the
ambiguous looking:
			vlan_id = bond->alb_info.current_alb_vlan;
			vlan_dev = __vlan_find_dev_deep(bond->dev,
							htons(ETH_P_8021Q),
							vlan_id);

			/* search for the next one, if not found - for any */
			if (vlan_dev)
				vlan_dev = __vlan_find_dev_next(bond->dev,
								vlan_dev);
			if (!vlan_dev)
				vlan_dev = __vlan_find_dev_next(bond->dev,
								NULL);

			if (vlan_dev) {
				vlan_id = vlan_dev_vlan_id(vlan_dev);
				bond->alb_info.current_alb_vlan = vlan_id;
			} else {
				bond->alb_info.current_alb_vlan = 0;
				rcu_read_unlock();
				kfree_skb(skb);
 				continue;
 			}

to something like (again untested, sorry for the style - line wrapping):

			vlan_id = bond->alb_info.current_alb_vlan+1;
			/* since from here is current+1, if there aren't any
			 * vlans up to current, it'll get current again if it's
			 * available
			 */
			dev_for_each_vlan_from(bond->dev, vlan_dev, htons(ETH_P_8021Q), vlan_id, i)
			break;

			if (vlan_dev) {
				vlan_id = vlan_dev_vlan_id(vlan_dev);
				bond->alb_info.current_alb_vlan = vlan_id;
			} else {
				bond->alb_info.current_alb_vlan = 0;
				rcu_read_unlock();
				kfree_skb(skb);
 				continue;
 			}

Cheers,
 Nik

Re: [PATCH v2 net-next 2/6] vlan: add __vlan_find_dev_next()

[Veaceslav Falico]

On Fri, Aug 09, 2013 at 01:07:31PM +0200, Nikolay Aleksandrov wrote:
...snip...
>We already discussed privately my comments about this new function, I'll leave
>them here just for the sake of having them documented:

As an almost top-posting - I'm currently on my vacation, so just a few
comments, without real code/patches. I'll come up with v3 at the next week,
hopefully.

>My proposition is to drop these changes altogether (the new function, and the
>vlan.h macro change) and instead add something like the following (I haven't
>tested it, it's only to illustrate the idea) in if_vlan.h so it'll be accessible
>to everyone:
>
>Version 1 (circular so you can simplify the bonding ALB code, I haven't left
>spaces because of auto-wrapping):
>
>#define dev_for_each_vlan_from(dev, vlandev, proto, from, i)
>	for (i=from+1, vlandev=__vlan_find_dev_deep(dev, proto, from); \
>	     i!=from; \
>	     i=(i+1)%VLAN_N_VID, vlandev=__vlan_find_dev_deep(dev, proto, i)) \
>		if (vlandev)
>
>#define dev_for_each_vlan(dev, vlandev, proto, i)
>	dev_for_each_vlan_from(dev, vlandev, proto, 0, i)

I like this idea, however it's an overkill in some places (and calling
8k times __vlan_find_dev_deep()->rcu_dereference, verify for vlan_info,
re-call itself 8k times if it's really a slave etc... not really good).
That was one of the main thoughts when I've chosen the other way -
	while (vlan = vlan_next(vlan))

They're also quite ugly (huge for, and an if () inside of a nested define)
- but that's more an IMO.

But, again, I like the dev_for_each_vlan() idea, and will look at it again,
trying to omit the __vlan_find_dev_deep() somehow and without that
monstrous for() :).

>
>Version 2 is the same but a little shorter, without the circular part.
>This way you reuse the already provided function __vlan_find_dev_deep and the
>churn is smaller, also __vlan_find_dev_deep takes care of the master issue (that
>is if dev doesn't have a vlan_info, then its master's vlan_info will be used).

Great ideas, thank you.

>Also the code will look much nicer IMO changing this:
>while ((vlan_dev = __vlan_find_dev_next))
>
>to
>
>dev_for_each_vlan(dev, vlan_dev, proto, i)

Yep, I agree, dev_for_each_vlan() looks a lot more readable. Maybe the
proto and i args can also be dropped somehow, I'll take a look...

>
>There're also 2 nice side-effects, first you'll only walk over 4096 entries (for
>the specified vlan proto only)

Not a big one, tbh, cause the current code looks for at most
vlan_id..8192(+0..vlan_id), so 8192. But the proto is indeed a good
side-effect. Another point - we're not working with QinQ at all...

>and the bonding ALB code will simplify from the
>ambiguous looking:
>			vlan_id = bond->alb_info.current_alb_vlan;
>			vlan_dev = __vlan_find_dev_deep(bond->dev,
>							htons(ETH_P_8021Q),
>							vlan_id);
>
>			/* search for the next one, if not found - for any */
>			if (vlan_dev)
>				vlan_dev = __vlan_find_dev_next(bond->dev,
>								vlan_dev);
>			if (!vlan_dev)
>				vlan_dev = __vlan_find_dev_next(bond->dev,
>								NULL);
>
>			if (vlan_dev) {
>				vlan_id = vlan_dev_vlan_id(vlan_dev);
>				bond->alb_info.current_alb_vlan = vlan_id;
>			} else {
>				bond->alb_info.current_alb_vlan = 0;
>				rcu_read_unlock();
>				kfree_skb(skb);
> 				continue;
> 			}
>
>to something like (again untested, sorry for the style - line wrapping):

Yep, that's why I really like the idea with dev_for_each_vlan(), with one
minor comment (which is fixable, I guess).

>
>			vlan_id = bond->alb_info.current_alb_vlan+1;
>			/* since from here is current+1, if there aren't any
>			 * vlans up to current, it'll get current again if it's
>			 * available
>			 */
>			dev_for_each_vlan_from(bond->dev, vlan_dev, htons(ETH_P_8021Q), vlan_id, i)
>			break;

			^^^^^ THIS. It makes a big "WHAT?" in my head. :)

>
>			if (vlan_dev) {
>				vlan_id = vlan_dev_vlan_id(vlan_dev);
>				bond->alb_info.current_alb_vlan = vlan_id;
>			} else {
>				bond->alb_info.current_alb_vlan = 0;
>				rcu_read_unlock();
>				kfree_skb(skb);
> 				continue;
> 			}
>
>Cheers,
> Nik

Thanks a lot for the feedback, really appreciated :).

[PATCH v2 net-next 3/6] bonding: make bond_alb use 8021q's dev->vlan_info instead of vlan_list

[Veaceslav Falico]

RFC -> v1: use the changed __vlan_find_dev_next, which now works with
	   vlan's net_device instead of vlan's id. Also, fix a subtle race
	   condition if we remove the only vlan while looping through
	   MAX_LP_BURST - we end up with using the old vlan_id, so set it
	   to 0 if we don't have vlans.
v1  -> v2: no change.

In alb mode, we only need each vlan's id (that is on top of bond) to tag
learning packets, so get them via __vlan_find_dev_next(bond->dev, last_dev).

We must also find *any* vlan (including last id stored in current_alb_vlan)
if we can't find anything >= current_alb_vlan id.

For that, we verify if bond has any vlans at all, and if yes - find the
next vlan id after current_alb_vlan id. So, if vlan id is not 0, we tag the
skb.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_alb.c |   47 ++++++++++++++++++++++++++++-----------
 drivers/net/bonding/bond_alb.h |    2 +-
 2 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 2684329..ced5753 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -974,8 +974,9 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
 {
 	struct bonding *bond = bond_get_bond_by_slave(slave);
 	struct learning_pkt pkt;
+	struct net_device *vlan_dev;
 	int size = sizeof(struct learning_pkt);
-	int i;
+	int i, vlan_id;
 
 	memset(&pkt, 0, size);
 	memcpy(pkt.mac_dst, mac_addr, ETH_ALEN);
@@ -1000,22 +1001,42 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
 		skb->priority = TC_PRIO_CONTROL;
 		skb->dev = slave->dev;
 
+		rcu_read_lock();
 		if (bond_vlan_used(bond)) {
-			struct vlan_entry *vlan;
-
-			vlan = bond_next_vlan(bond,
-					      bond->alb_info.current_alb_vlan);
-
-			bond->alb_info.current_alb_vlan = vlan;
-			if (!vlan) {
+			/* first try to find the previously used vlan by
+			 * id, which might have gone away already
+			 */
+			vlan_id = bond->alb_info.current_alb_vlan;
+			vlan_dev = __vlan_find_dev_deep(bond->dev,
+							htons(ETH_P_8021Q),
+							vlan_id);
+
+			/* search for the next one, if not found - for any */
+			if (vlan_dev)
+				vlan_dev = __vlan_find_dev_next(bond->dev,
+								vlan_dev);
+			if (!vlan_dev)
+				vlan_dev = __vlan_find_dev_next(bond->dev,
+								NULL);
+
+			if (vlan_dev) {
+				vlan_id = vlan_dev_vlan_id(vlan_dev);
+				bond->alb_info.current_alb_vlan = vlan_id;
+			} else {
+				bond->alb_info.current_alb_vlan = 0;
+				rcu_read_unlock();
 				kfree_skb(skb);
 				continue;
 			}
+		} else
+			vlan_id = 0;
+		rcu_read_unlock();
 
-			skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan->vlan_id);
+		if (vlan_id) {
+			skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_id);
 			if (!skb) {
-				pr_err("%s: Error: failed to insert VLAN tag\n",
-				       bond->dev->name);
+				pr_err("%s: Error: failed to insert VLAN tag %d\n",
+				       bond->dev->name, vlan_id);
 				continue;
 			}
 		}
@@ -1759,8 +1780,8 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
 {
 	if (bond->alb_info.current_alb_vlan &&
-	    (bond->alb_info.current_alb_vlan->vlan_id == vlan_id)) {
-		bond->alb_info.current_alb_vlan = NULL;
+	    (bond->alb_info.current_alb_vlan == vlan_id)) {
+		bond->alb_info.current_alb_vlan = 0;
 	}
 
 	if (bond->alb_info.rlb_enabled) {
diff --git a/drivers/net/bonding/bond_alb.h b/drivers/net/bonding/bond_alb.h
index e7a5b8b..b2452aa 100644
--- a/drivers/net/bonding/bond_alb.h
+++ b/drivers/net/bonding/bond_alb.h
@@ -170,7 +170,7 @@ struct alb_bond_info {
 						 * rx traffic should be
 						 * rebalanced
 						 */
-	struct vlan_entry	*current_alb_vlan;
+	u16			current_alb_vlan;
 };
 
 int bond_alb_initialize(struct bonding *bond, int rlb_enabled);
-- 
1.7.1

Re: [PATCH v2 net-next 3/6] bonding: make bond_alb use 8021q's dev->vlan_info instead of vlan_list

[Nikolay Aleksandrov]

On 08/08/2013 06:57 PM, Veaceslav Falico wrote:
> RFC -> v1: use the changed __vlan_find_dev_next, which now works with
> 	   vlan's net_device instead of vlan's id. Also, fix a subtle race
> 	   condition if we remove the only vlan while looping through
> 	   MAX_LP_BURST - we end up with using the old vlan_id, so set it
> 	   to 0 if we don't have vlans.
> v1  -> v2: no change.
> 
> In alb mode, we only need each vlan's id (that is on top of bond) to tag
> learning packets, so get them via __vlan_find_dev_next(bond->dev, last_dev).
> 
> We must also find *any* vlan (including last id stored in current_alb_vlan)
> if we can't find anything >= current_alb_vlan id.
> 
> For that, we verify if bond has any vlans at all, and if yes - find the
> next vlan id after current_alb_vlan id. So, if vlan id is not 0, we tag the
> skb.
> 
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>  drivers/net/bonding/bond_alb.c |   47 ++++++++++++++++++++++++++++-----------
>  drivers/net/bonding/bond_alb.h |    2 +-
>  2 files changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 2684329..ced5753 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -974,8 +974,9 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
>  {
>  	struct bonding *bond = bond_get_bond_by_slave(slave);
>  	struct learning_pkt pkt;
> +	struct net_device *vlan_dev;
>  	int size = sizeof(struct learning_pkt);
> -	int i;
> +	int i, vlan_id;
Styling nitpick: maybe re-arrange them longest -> shortest ?

>  
>  	memset(&pkt, 0, size);
>  	memcpy(pkt.mac_dst, mac_addr, ETH_ALEN);
> @@ -1000,22 +1001,42 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
>  		skb->priority = TC_PRIO_CONTROL;
>  		skb->dev = slave->dev;
>  
> +		rcu_read_lock();
Would've saved a couple of lines if we could move the rcu_read_lock inside the
if {} block but I know that you're keeping the bond_vlan_used() result here, but
I think that with the new functions we're covered i.e. if the vlan has
disappeared between the call to bond_vlan_used and the afterwards we'll get NULL
and just drop the packet, but this can be reworked to continue with the packet
without a vlan if one is not found that is to send it without tag :-)
>  		if (bond_vlan_used(bond)) {
> -			struct vlan_entry *vlan;
> -
> -			vlan = bond_next_vlan(bond,
> -					      bond->alb_info.current_alb_vlan);
> -
> -			bond->alb_info.current_alb_vlan = vlan;
> -			if (!vlan) {
> +			/* first try to find the previously used vlan by
> +			 * id, which might have gone away already
> +			 */
> +			vlan_id = bond->alb_info.current_alb_vlan;
> +			vlan_dev = __vlan_find_dev_deep(bond->dev,
> +							htons(ETH_P_8021Q),
> +							vlan_id);
> +
> +			/* search for the next one, if not found - for any */
> +			if (vlan_dev)
> +				vlan_dev = __vlan_find_dev_next(bond->dev,
> +								vlan_dev);
> +			if (!vlan_dev)
> +				vlan_dev = __vlan_find_dev_next(bond->dev,
> +								NULL);
> +
This part really looks ambiguous, I've left a comment that concerns it in my
reply to patch 02.

> +			if (vlan_dev) {
> +				vlan_id = vlan_dev_vlan_id(vlan_dev);
> +				bond->alb_info.current_alb_vlan = vlan_id;
> +			} else {
> +				bond->alb_info.current_alb_vlan = 0;
> +				rcu_read_unlock();
>  				kfree_skb(skb);
>  				continue;
>  			}
> +		} else
> +			vlan_id = 0;
Styling nitpick: if {} else {}

Cheers,
 Nik

Re: [PATCH v2 net-next 3/6] bonding: make bond_alb use 8021q's dev->vlan_info instead of vlan_list

[Veaceslav Falico]

On Fri, Aug 09, 2013 at 01:13:58PM +0200, Nikolay Aleksandrov wrote:
>On 08/08/2013 06:57 PM, Veaceslav Falico wrote:
>> RFC -> v1: use the changed __vlan_find_dev_next, which now works with
>> 	   vlan's net_device instead of vlan's id. Also, fix a subtle race
>> 	   condition if we remove the only vlan while looping through
>> 	   MAX_LP_BURST - we end up with using the old vlan_id, so set it
>> 	   to 0 if we don't have vlans.
>> v1  -> v2: no change.
>>
>> In alb mode, we only need each vlan's id (that is on top of bond) to tag
>> learning packets, so get them via __vlan_find_dev_next(bond->dev, last_dev).
>>
>> We must also find *any* vlan (including last id stored in current_alb_vlan)
>> if we can't find anything >= current_alb_vlan id.
>>
>> For that, we verify if bond has any vlans at all, and if yes - find the
>> next vlan id after current_alb_vlan id. So, if vlan id is not 0, we tag the
>> skb.
>>
>> CC: Jay Vosburgh <fubar@us.ibm.com>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
>> ---
>>  drivers/net/bonding/bond_alb.c |   47 ++++++++++++++++++++++++++++-----------
>>  drivers/net/bonding/bond_alb.h |    2 +-
>>  2 files changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index 2684329..ced5753 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -974,8 +974,9 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
>>  {
>>  	struct bonding *bond = bond_get_bond_by_slave(slave);
>>  	struct learning_pkt pkt;
>> +	struct net_device *vlan_dev;
>>  	int size = sizeof(struct learning_pkt);
>> -	int i;
>> +	int i, vlan_id;
>Styling nitpick: maybe re-arrange them longest -> shortest ?

Yep, sure, thank you.

>
>>
>>  	memset(&pkt, 0, size);
>>  	memcpy(pkt.mac_dst, mac_addr, ETH_ALEN);
>> @@ -1000,22 +1001,42 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
>>  		skb->priority = TC_PRIO_CONTROL;
>>  		skb->dev = slave->dev;
>>
>> +		rcu_read_lock();
>Would've saved a couple of lines if we could move the rcu_read_lock inside the
>if {} block but I know that you're keeping the bond_vlan_used() result here, but
>I think that with the new functions we're covered i.e. if the vlan has
>disappeared between the call to bond_vlan_used and the afterwards we'll get NULL
>and just drop the packet, but this can be reworked to continue with the packet
>without a vlan if one is not found that is to send it without tag :-)

Yeah, not a lot of sense to hold it, however I don't see right now how it
might save us lines - we need to hold vlan_dev if it's found, and need to
release the lock if it's NULL (before continue).

>>  		if (bond_vlan_used(bond)) {
>> -			struct vlan_entry *vlan;
>> -
>> -			vlan = bond_next_vlan(bond,
>> -					      bond->alb_info.current_alb_vlan);
>> -
>> -			bond->alb_info.current_alb_vlan = vlan;
>> -			if (!vlan) {
>> +			/* first try to find the previously used vlan by
>> +			 * id, which might have gone away already
>> +			 */
>> +			vlan_id = bond->alb_info.current_alb_vlan;
>> +			vlan_dev = __vlan_find_dev_deep(bond->dev,
>> +							htons(ETH_P_8021Q),
>> +							vlan_id);
>> +
>> +			/* search for the next one, if not found - for any */
>> +			if (vlan_dev)
>> +				vlan_dev = __vlan_find_dev_next(bond->dev,
>> +								vlan_dev);
>> +			if (!vlan_dev)
>> +				vlan_dev = __vlan_find_dev_next(bond->dev,
>> +								NULL);
>> +
>This part really looks ambiguous, I've left a comment that concerns it in my
>reply to patch 02.

Yep, seen that, it indeed is not quite clear (though working when you think
a bit about it :) ). I'll address your comment about 02/06 in several days,
I'm travelling now.

>
>> +			if (vlan_dev) {
>> +				vlan_id = vlan_dev_vlan_id(vlan_dev);
>> +				bond->alb_info.current_alb_vlan = vlan_id;
>> +			} else {
>> +				bond->alb_info.current_alb_vlan = 0;
>> +				rcu_read_unlock();
>>  				kfree_skb(skb);
>>  				continue;
>>  			}
>> +		} else
>> +			vlan_id = 0;
>Styling nitpick: if {} else {}

Yep, thanks.

>
>Cheers,
> Nik

[PATCH v2 net-next 4/6] bonding: convert bond_has_this_ip to use bond->dev->vlan_info

[Veaceslav Falico]

RFC -> v1: use the new __vlan_find_dev_next(), which simplifies the code
	   and omits issues with vlan id 0.
v1  -> v2: fix the use of uninitialized vlan_dev

Use __vlan_find_dev_next() to loop through dev's vlan devices and verify if
the ip matches.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 9d1045d..e52e2d5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2392,20 +2392,18 @@ re_arm:
 
 static int bond_has_this_ip(struct bonding *bond, __be32 ip)
 {
-	struct vlan_entry *vlan;
-	struct net_device *vlan_dev;
+	struct net_device *vlan_dev = NULL;
 
 	if (ip == bond_confirm_addr(bond->dev, 0, ip))
 		return 1;
 
-	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
-		rcu_read_lock();
-		vlan_dev = __vlan_find_dev_deep(bond->dev, htons(ETH_P_8021Q),
-						vlan->vlan_id);
-		rcu_read_unlock();
-		if (vlan_dev && ip == bond_confirm_addr(vlan_dev, 0, ip))
+	rcu_read_lock();
+	while ((vlan_dev = __vlan_find_dev_next(bond->dev, vlan_dev)))
+		if (ip == bond_confirm_addr(vlan_dev, 0, ip)) {
+			rcu_read_unlock();
 			return 1;
-	}
+		}
+	rcu_read_unlock();
 
 	return 0;
 }
-- 
1.7.1

[PATCH v2 net-next 5/6] bonding: convert bond_arp_send_all to use bond->dev->vlan_info

[Veaceslav Falico]

RFC -> v1: use the new __vlan_find_dev_next(), also release rcu_read_lock()
	   only after we stop using the vlan_dev.
v1  -> v2: no change.

Instead of looping through bond->vlan_list, loop through
bond->dev->vlan_info via __vlan_find_dev_next() under rcu_read_lock().

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c |   29 +++++++++++++----------------
 1 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e52e2d5..f536d05 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2440,11 +2440,10 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, __be32 dest_
 
 static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 {
-	int i, vlan_id;
-	__be32 *targets = bond->params.arp_targets;
-	struct vlan_entry *vlan;
-	struct net_device *vlan_dev = NULL;
+	struct net_device *vlan_dev;
 	struct rtable *rt;
+	__be32 *targets = bond->params.arp_targets;
+	int i;
 
 	for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
 		__be32 addr;
@@ -2486,28 +2485,26 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 			continue;
 		}
 
-		vlan_id = 0;
-		list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
-			rcu_read_lock();
-			vlan_dev = __vlan_find_dev_deep(bond->dev,
-							htons(ETH_P_8021Q),
-							vlan->vlan_id);
-			rcu_read_unlock();
+		vlan_dev = NULL;
+
+		rcu_read_lock();
+		while ((vlan_dev = __vlan_find_dev_next(bond->dev, vlan_dev)))
 			if (vlan_dev == rt->dst.dev) {
-				vlan_id = vlan->vlan_id;
 				pr_debug("basa: vlan match on %s %d\n",
-				       vlan_dev->name, vlan_id);
+					 vlan_dev->name,
+					 vlan_dev_vlan_id(vlan_dev));
 				break;
 			}
-		}
 
-		if (vlan_id && vlan_dev) {
+		if (vlan_dev) {
 			ip_rt_put(rt);
 			addr = bond_confirm_addr(vlan_dev, targets[i], 0);
 			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
-				      addr, vlan_id);
+				      addr, vlan_dev_vlan_id(vlan_dev));
+			rcu_read_unlock();
 			continue;
 		}
+		rcu_read_unlock();
 
 		if (net_ratelimit()) {
 			pr_warning("%s: no path to arp_ip_target %pI4 via rt.dev %s\n",
-- 
1.7.1

Re: [PATCH v2 net-next 5/6] bonding: convert bond_arp_send_all to use bond->dev->vlan_info

[Nikolay Aleksandrov]

On 08/08/2013 06:57 PM, Veaceslav Falico wrote:
> RFC -> v1: use the new __vlan_find_dev_next(), also release rcu_read_lock()
> 	   only after we stop using the vlan_dev.
> v1  -> v2: no change.
> 
> Instead of looping through bond->vlan_list, loop through
> bond->dev->vlan_info via __vlan_find_dev_next() under rcu_read_lock().
> 
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>  drivers/net/bonding/bond_main.c |   29 +++++++++++++----------------
>  1 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index e52e2d5..f536d05 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2440,11 +2440,10 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op, __be32 dest_
>  
>  static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>  {
> -	int i, vlan_id;
> -	__be32 *targets = bond->params.arp_targets;
> -	struct vlan_entry *vlan;
> -	struct net_device *vlan_dev = NULL;
> +	struct net_device *vlan_dev;
>  	struct rtable *rt;
> +	__be32 *targets = bond->params.arp_targets;
> +	int i;
>  
Style nitpick: maybe move them longest -> shortest.

>  	for (i = 0; (i < BOND_MAX_ARP_TARGETS); i++) {
>  		__be32 addr;
> @@ -2486,28 +2485,26 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>  			continue;
>  		}
>  
> -		vlan_id = 0;
> -		list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
> -			rcu_read_lock();
> -			vlan_dev = __vlan_find_dev_deep(bond->dev,
> -							htons(ETH_P_8021Q),
> -							vlan->vlan_id);
> -			rcu_read_unlock();
> +		vlan_dev = NULL;
> +
> +		rcu_read_lock();
> +		while ((vlan_dev = __vlan_find_dev_next(bond->dev, vlan_dev)))
>  			if (vlan_dev == rt->dst.dev) {
> -				vlan_id = vlan->vlan_id;
>  				pr_debug("basa: vlan match on %s %d\n",
> -				       vlan_dev->name, vlan_id);
> +					 vlan_dev->name,
> +					 vlan_dev_vlan_id(vlan_dev));
>  				break;
>  			}
> -		}
>  
> -		if (vlan_id && vlan_dev) {
> +		if (vlan_dev) {
>  			ip_rt_put(rt);
>  			addr = bond_confirm_addr(vlan_dev, targets[i], 0);
>  			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
> -				      addr, vlan_id);
> +				      addr, vlan_dev_vlan_id(vlan_dev));
> +			rcu_read_unlock();
>  			continue;
>  		}
> +		rcu_read_unlock();
>
I think these lines can be re-arranged to something shorter like:

                rcu_read_lock();
                vlan_dev = NULL;
                while ((vlan_dev = __vlan_find_dev_next(bond->dev, vlan_dev)))
                        if (vlan_dev == rt->dst.dev) {
                                pr_debug("basa: vlan match on %s %d\n",
                                         vlan_dev->name,
                                         vlan_dev_vlan_id(vlan_dev));
                                break;
                        }

                if (vlan_dev) {
                        ip_rt_put(rt);
                        addr = bond_confirm_addr(vlan_dev, targets[i], 0);
                        bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
                                      addr, vlan_dev_vlan_id(vlan_dev));
                } else {
			if (net_ratelimit())
                        	pr_warning("%s: no path to arp_ip_target %pI4 via
rt.dev %s\n",
                                	   bond->dev->name, &targets[i],
                                   	rt->dst.dev ? rt->dst.dev->name : "NULL");
	               ip_rt_put(rt);	
                }
                rcu_read_unlock();

But either way is fine, I just think this one is more readable.

Cheers,
 Nik

[PATCH v2 net-next 6/6] bonding: remove unused bond->vlan_list

[Veaceslav Falico]

RFC -> v1: No changes.
v1  -> v2: No changes.

There are currently no users of bond->vlan_list (other than the maintaining
functions add/remove) - so remove it and every unneeded helper.

In this patch we remove:
vlan_list from struct bonding
bond_next_vlan - we don't need it anymore
struct vlan_entry - it was a helper struct for bond->vlan_list
some bits from bond_vlan_rx_add/kill_vid() - which were related to
	bond->vlan_list
(de)initialization of vlan_list from bond_setup/uninit
bond_add_vlan - its only scope was to maintain bond->vlan_list

We don't fully remove bond_del_vlan() - bond_alb_clear_vlan() still needs
to be called when a vlan disappears. And we make bond_del_vlan() to not
return anything cause it cannot fail already.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c |  117 +-------------------------------------
 drivers/net/bonding/bonding.h   |    6 --
 2 files changed, 4 insertions(+), 119 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f536d05..6df818b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -282,113 +282,24 @@ const char *bond_mode_name(int mode)
 /*---------------------------------- VLAN -----------------------------------*/
 
 /**
- * bond_add_vlan - add a new vlan id on bond
- * @bond: bond that got the notification
- * @vlan_id: the vlan id to add
- *
- * Returns -ENOMEM if allocation failed.
- */
-static int bond_add_vlan(struct bonding *bond, unsigned short vlan_id)
-{
-	struct vlan_entry *vlan;
-
-	pr_debug("bond: %s, vlan id %d\n",
-		 (bond ? bond->dev->name : "None"), vlan_id);
-
-	vlan = kzalloc(sizeof(struct vlan_entry), GFP_KERNEL);
-	if (!vlan)
-		return -ENOMEM;
-
-	INIT_LIST_HEAD(&vlan->vlan_list);
-	vlan->vlan_id = vlan_id;
-
-	write_lock_bh(&bond->lock);
-
-	list_add_tail(&vlan->vlan_list, &bond->vlan_list);
-
-	write_unlock_bh(&bond->lock);
-
-	pr_debug("added VLAN ID %d on bond %s\n", vlan_id, bond->dev->name);
-
-	return 0;
-}
-
-/**
  * bond_del_vlan - delete a vlan id from bond
  * @bond: bond that got the notification
  * @vlan_id: the vlan id to delete
  *
  * returns -ENODEV if @vlan_id was not found in @bond.
  */
-static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
+static void bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
 {
-	struct vlan_entry *vlan;
-	int res = -ENODEV;
-
 	pr_debug("bond: %s, vlan id %d\n", bond->dev->name, vlan_id);
 
 	block_netpoll_tx();
 	write_lock_bh(&bond->lock);
 
-	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
-		if (vlan->vlan_id == vlan_id) {
-			list_del(&vlan->vlan_list);
-
-			if (bond_is_lb(bond))
-				bond_alb_clear_vlan(bond, vlan_id);
-
-			pr_debug("removed VLAN ID %d from bond %s\n",
-				 vlan_id, bond->dev->name);
+	if (bond_is_lb(bond))
+		bond_alb_clear_vlan(bond, vlan_id);
 
-			kfree(vlan);
-
-			res = 0;
-			goto out;
-		}
-	}
-
-	pr_debug("couldn't find VLAN ID %d in bond %s\n",
-		 vlan_id, bond->dev->name);
-
-out:
 	write_unlock_bh(&bond->lock);
 	unblock_netpoll_tx();
-	return res;
-}
-
-/**
- * bond_next_vlan - safely skip to the next item in the vlans list.
- * @bond: the bond we're working on
- * @curr: item we're advancing from
- *
- * Returns %NULL if list is empty, bond->next_vlan if @curr is %NULL,
- * or @curr->next otherwise (even if it is @curr itself again).
- *
- * Caller must hold bond->lock
- */
-struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr)
-{
-	struct vlan_entry *next, *last;
-
-	if (list_empty(&bond->vlan_list))
-		return NULL;
-
-	if (!curr) {
-		next = list_entry(bond->vlan_list.next,
-				  struct vlan_entry, vlan_list);
-	} else {
-		last = list_entry(bond->vlan_list.prev,
-				  struct vlan_entry, vlan_list);
-		if (last == curr) {
-			next = list_entry(bond->vlan_list.next,
-					  struct vlan_entry, vlan_list);
-		} else {
-			next = list_entry(curr->vlan_list.next,
-					  struct vlan_entry, vlan_list);
-		}
-	}
-
-	return next;
 }
 
 /**
@@ -450,13 +361,6 @@ static int bond_vlan_rx_add_vid(struct net_device *bond_dev,
 			goto unwind;
 	}
 
-	res = bond_add_vlan(bond, vid);
-	if (res) {
-		pr_err("%s: Error: Failed to add vlan id %d\n",
-		       bond_dev->name, vid);
-		goto unwind;
-	}
-
 	return 0;
 
 unwind:
@@ -477,17 +381,11 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct slave *slave;
-	int res;
 
 	bond_for_each_slave(bond, slave)
 		vlan_vid_del(slave->dev, proto, vid);
 
-	res = bond_del_vlan(bond, vid);
-	if (res) {
-		pr_err("%s: Error: Failed to remove vlan id %d\n",
-		       bond_dev->name, vid);
-		return res;
-	}
+	bond_del_vlan(bond, vid);
 
 	return 0;
 }
@@ -4135,7 +4033,6 @@ static void bond_setup(struct net_device *bond_dev)
 
 	/* Initialize pointers */
 	bond->dev = bond_dev;
-	INIT_LIST_HEAD(&bond->vlan_list);
 
 	/* Initialize the device entry points */
 	ether_setup(bond_dev);
@@ -4188,7 +4085,6 @@ static void bond_uninit(struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct slave *slave, *tmp_slave;
-	struct vlan_entry *vlan, *tmp;
 
 	bond_netpoll_cleanup(bond_dev);
 
@@ -4200,11 +4096,6 @@ static void bond_uninit(struct net_device *bond_dev)
 	list_del(&bond->bond_list);
 
 	bond_debug_unregister(bond);
-
-	list_for_each_entry_safe(vlan, tmp, &bond->vlan_list, vlan_list) {
-		list_del(&vlan->vlan_list);
-		kfree(vlan);
-	}
 }
 
 /*------------------------- Module initialization ---------------------------*/
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 9c4539e..bfa33c4 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -186,11 +186,6 @@ struct bond_parm_tbl {
 
 #define BOND_MAX_MODENAME_LEN 20
 
-struct vlan_entry {
-	struct list_head vlan_list;
-	unsigned short vlan_id;
-};
-
 struct slave {
 	struct net_device *dev; /* first - useful for panic debug */
 	struct list_head list;
@@ -255,7 +250,6 @@ struct bonding {
 	struct   ad_bond_info ad_info;
 	struct   alb_bond_info alb_info;
 	struct   bond_params params;
-	struct   list_head vlan_list;
 	struct   workqueue_struct *wq;
 	struct   delayed_work mii_work;
 	struct   delayed_work arp_work;
-- 
1.7.1

Re: [PATCH v2 net-next 6/6] bonding: remove unused bond->vlan_list

[Nikolay Aleksandrov]

On 08/08/2013 06:57 PM, Veaceslav Falico wrote:
> RFC -> v1: No changes.
> v1  -> v2: No changes.
> 
> There are currently no users of bond->vlan_list (other than the maintaining
> functions add/remove) - so remove it and every unneeded helper.
> 
> In this patch we remove:
> vlan_list from struct bonding
> bond_next_vlan - we don't need it anymore
> struct vlan_entry - it was a helper struct for bond->vlan_list
> some bits from bond_vlan_rx_add/kill_vid() - which were related to
> 	bond->vlan_list
> (de)initialization of vlan_list from bond_setup/uninit
> bond_add_vlan - its only scope was to maintain bond->vlan_list
> 
> We don't fully remove bond_del_vlan() - bond_alb_clear_vlan() still needs
> to be called when a vlan disappears. And we make bond_del_vlan() to not
> return anything cause it cannot fail already.
> 
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---

Very much
Acked-by: Nikolay Aleksandrov <nikolay@redhat.com>

Re: [PATCH net-next v2 0/6] bonding: remove bond->vlan_list

[Veaceslav Falico]

On Thu, Aug 08, 2013 at 06:57:53PM +0200, Veaceslav Falico wrote:
>RFC -> v1: Got some feedback from Nikolay Aleksandrov (privately), tried to
>	   address it, also fixed some bugs that I've found on the way. I
>	   think it's ready to be considered a patchset for
>	   review/inclusion in net-next.
>
>v1  -> v2: Remove ASSERT_RTNL() from vlan_uses_dev(), cause it can be
>	   already called under rcu, without rtnl. Don't check for master
>	   device in __vlan_find_dev_next(), otherwise we won't be able to
>	   work in situations when a device has both vlans and master
>	   device. Properly init vlan_dev in bond_has_this_ip() before
>	   using (sigh). There was a proposal of making a macro
>	   "dev_for_each_vlan_from(dev, vlan_dev, i, from)", which would
>	   use __vlan_find_dev_deep() inside, with its strong and weak
>	   parts, but I've decided to stick to the "while (dev = next())"
>	   scheme currently - it might be added anytime, and now the only
>	   user (bonding) doesn't really need it.

I've taken a different (less intrusive for non-bonding stuff) approach on
the issue, so sent the new patchset not as a v3, but as a standalone one:

[PATCH net-next 0/8] bonding: remove vlan special handling

Thanks all for the help.