[PATCH net 1/2] vlan: make vlan_dev_real_dev work over stacked vlans

[PATCH net 1/2] vlan: make vlan_dev_real_dev work over stacked vlans

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>

Sometimes we might have stacked vlans on top of each other, and we're
interested in the first non-vlan real device on the path, so transform
vlan_dev_real_dev to go over the stacked vlans and extract the first
non-vlan device.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 net/8021q/vlan_core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 4a78c4d..6ee48aa 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -91,7 +91,12 @@ EXPORT_SYMBOL(__vlan_find_dev_deep);
 
 struct net_device *vlan_dev_real_dev(const struct net_device *dev)
 {
-	return vlan_dev_priv(dev)->real_dev;
+	struct net_device *ret = vlan_dev_priv(dev)->real_dev;
+
+	while (is_vlan_dev(ret))
+		ret = vlan_dev_priv(ret)->real_dev;
+
+	return ret;
 }
 EXPORT_SYMBOL(vlan_dev_real_dev);
 
-- 
1.8.1.4

[PATCH net v2 2/2] net_sched: make dev_trans_start return vlan's real dev trans_start

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>

Vlan devices are LLTX and don't update their own trans_start, so if
dev_trans_start has to be called with a vlan device then 0 or a stale
value will be returned. Currently the bonding is the only such user, and
it's needed for proper arp monitoring when the slaves are vlans.
Fix this by extracting the vlan's real device trans_start.

Suggested-by: David Miller <davem@davemloft.net>
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
v2: drop the while, since vlan_dev_real_dev now does it for us

 net/sched/sch_generic.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 4626cef..eeb8276 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -25,6 +25,7 @@
 #include <linux/rcupdate.h>
 #include <linux/list.h>
 #include <linux/slab.h>
+#include <linux/if_vlan.h>
 #include <net/sch_generic.h>
 #include <net/pkt_sched.h>
 #include <net/dst.h>
@@ -207,15 +208,19 @@ void __qdisc_run(struct Qdisc *q)
 
 unsigned long dev_trans_start(struct net_device *dev)
 {
-	unsigned long val, res = dev->trans_start;
+	unsigned long val, res;
 	unsigned int i;
 
+	if (is_vlan_dev(dev))
+		dev = vlan_dev_real_dev(dev);
+	res = dev->trans_start;
 	for (i = 0; i < dev->num_tx_queues; i++) {
 		val = netdev_get_tx_queue(dev, i)->trans_start;
 		if (val && time_after(val, res))
 			res = val;
 	}
 	dev->trans_start = res;
+
 	return res;
 }
 EXPORT_SYMBOL(dev_trans_start);
-- 
1.8.1.4

Re: [PATCH net 1/2] vlan: make vlan_dev_real_dev work over stacked vlans

[Veaceslav Falico]

On Sat, Aug 03, 2013 at 10:07:46PM +0200, Nikolay Aleksandrov wrote:
>From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
>
>Sometimes we might have stacked vlans on top of each other, and we're
>interested in the first non-vlan real device on the path, so transform
>vlan_dev_real_dev to go over the stacked vlans and extract the first
>non-vlan device.
>
>Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

>---
> net/8021q/vlan_core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>index 4a78c4d..6ee48aa 100644
>--- a/net/8021q/vlan_core.c
>+++ b/net/8021q/vlan_core.c
>@@ -91,7 +91,12 @@ EXPORT_SYMBOL(__vlan_find_dev_deep);
>
> struct net_device *vlan_dev_real_dev(const struct net_device *dev)
> {
>-	return vlan_dev_priv(dev)->real_dev;
>+	struct net_device *ret = vlan_dev_priv(dev)->real_dev;
>+
>+	while (is_vlan_dev(ret))
>+		ret = vlan_dev_priv(ret)->real_dev;
>+
>+	return ret;
> }
> EXPORT_SYMBOL(vlan_dev_real_dev);
>
>-- 
>1.8.1.4
>

Re: [PATCH net v2 2/2] net_sched: make dev_trans_start return vlan's real dev trans_start

[Veaceslav Falico]

On Sat, Aug 03, 2013 at 10:07:47PM +0200, Nikolay Aleksandrov wrote:
>From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
>
>Vlan devices are LLTX and don't update their own trans_start, so if
>dev_trans_start has to be called with a vlan device then 0 or a stale
>value will be returned. Currently the bonding is the only such user, and
>it's needed for proper arp monitoring when the slaves are vlans.
>Fix this by extracting the vlan's real device trans_start.
>
>Suggested-by: David Miller <davem@davemloft.net>
>Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

Acked-by: Veaceslav Falico <vfalico@redhat.com>

>---
>v2: drop the while, since vlan_dev_real_dev now does it for us
>
> net/sched/sch_generic.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>index 4626cef..eeb8276 100644
>--- a/net/sched/sch_generic.c
>+++ b/net/sched/sch_generic.c
>@@ -25,6 +25,7 @@
> #include <linux/rcupdate.h>
> #include <linux/list.h>
> #include <linux/slab.h>
>+#include <linux/if_vlan.h>
> #include <net/sch_generic.h>
> #include <net/pkt_sched.h>
> #include <net/dst.h>
>@@ -207,15 +208,19 @@ void __qdisc_run(struct Qdisc *q)
>
> unsigned long dev_trans_start(struct net_device *dev)
> {
>-	unsigned long val, res = dev->trans_start;
>+	unsigned long val, res;
> 	unsigned int i;
>
>+	if (is_vlan_dev(dev))
>+		dev = vlan_dev_real_dev(dev);
>+	res = dev->trans_start;
> 	for (i = 0; i < dev->num_tx_queues; i++) {
> 		val = netdev_get_tx_queue(dev, i)->trans_start;
> 		if (val && time_after(val, res))
> 			res = val;
> 	}
> 	dev->trans_start = res;
>+
> 	return res;
> }
> EXPORT_SYMBOL(dev_trans_start);
>-- 
>1.8.1.4
>

Re: [PATCH net 1/2] vlan: make vlan_dev_real_dev work over stacked vlans

[Nikolay Aleksandrov]

On 08/03/2013 10:07 PM, Nikolay Aleksandrov wrote:
> From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
> 
> Sometimes we might have stacked vlans on top of each other, and we're
> interested in the first non-vlan real device on the path, so transform
> vlan_dev_real_dev to go over the stacked vlans and extract the first
> non-vlan device.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---
>  net/8021q/vlan_core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 4a78c4d..6ee48aa 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -91,7 +91,12 @@ EXPORT_SYMBOL(__vlan_find_dev_deep);
>  
>  struct net_device *vlan_dev_real_dev(const struct net_device *dev)
>  {
> -	return vlan_dev_priv(dev)->real_dev;
> +	struct net_device *ret = vlan_dev_priv(dev)->real_dev;
> +
> +	while (is_vlan_dev(ret))
> +		ret = vlan_dev_priv(ret)->real_dev;
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(vlan_dev_real_dev);
>  
> 
I have one question - why not make it possible to call vlan_dev_real_dev()
with any device (vlan/non-vlan) and make it return a real device in all
cases (e.g. if it's a non-vlan device simply return it) ?
In the terms of the code above - simply change:
struct net_device *ret = vlan_dev_priv(dev)->real_dev;
with
struct net_device *ret = dev;

This way we'll be able to simplify calls like if (is_vlan_dev(dev)) (or the
flag check alternative of this) to just vlan_dev_real_dev(). The old call
sites of this function will still work properly.

Nik

Re: [PATCH net 1/2] vlan: make vlan_dev_real_dev work over stacked vlans

[Veaceslav Falico]

On Sun, Aug 04, 2013 at 04:17:43AM +0200, Nikolay Aleksandrov wrote:
>On 08/03/2013 10:07 PM, Nikolay Aleksandrov wrote:
>> From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
>>
>> Sometimes we might have stacked vlans on top of each other, and we're
>> interested in the first non-vlan real device on the path, so transform
>> vlan_dev_real_dev to go over the stacked vlans and extract the first
>> non-vlan device.
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>> ---
>>  net/8021q/vlan_core.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index 4a78c4d..6ee48aa 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -91,7 +91,12 @@ EXPORT_SYMBOL(__vlan_find_dev_deep);
>>
>>  struct net_device *vlan_dev_real_dev(const struct net_device *dev)
>>  {
>> -	return vlan_dev_priv(dev)->real_dev;
>> +	struct net_device *ret = vlan_dev_priv(dev)->real_dev;
>> +
>> +	while (is_vlan_dev(ret))
>> +		ret = vlan_dev_priv(ret)->real_dev;
>> +
>> +	return ret;
>>  }
>>  EXPORT_SYMBOL(vlan_dev_real_dev);
>>
>>
>I have one question - why not make it possible to call vlan_dev_real_dev()
>with any device (vlan/non-vlan) and make it return a real device in all
>cases (e.g. if it's a non-vlan device simply return it) ?
>In the terms of the code above - simply change:
>struct net_device *ret = vlan_dev_priv(dev)->real_dev;
>with
>struct net_device *ret = dev;
>
>This way we'll be able to simplify calls like if (is_vlan_dev(dev)) (or the
>flag check alternative of this) to just vlan_dev_real_dev(). The old call
>sites of this function will still work properly.

I think it'll be a bit harder to understand the callers' code. Now it
always has the logic "wait, we might be having a vlan here, so lets verify
if it's really a vlan and if yes - get the real device". However with this
approach it might look like "we *really* have a vlan here - so lets get the
real device" (because of the function name - vlan_dev_real_dev) - which is
wrong. And it doesn't really provide any speed/size improvements - so it's
kind of useless.

So, unless a better function name can be found (which I can't come up with
- dev_or_strip_vlan()? dev_devlanitize()? :) ) - I'd stay with the current
  version. Though, again, I don't have a strong opinion on this.

>
>Nik

Re: [PATCH net 1/2] vlan: make vlan_dev_real_dev work over stacked vlans

[Nikolay Aleksandrov]

On 08/05/2013 08:58 AM, Veaceslav Falico wrote:
> On Sun, Aug 04, 2013 at 04:17:43AM +0200, Nikolay Aleksandrov wrote:
>> On 08/03/2013 10:07 PM, Nikolay Aleksandrov wrote:
<snip>
>> I have one question - why not make it possible to call vlan_dev_real_dev()
>> with any device (vlan/non-vlan) and make it return a real device in all
>> cases (e.g. if it's a non-vlan device simply return it) ?
>> In the terms of the code above - simply change:
>> struct net_device *ret = vlan_dev_priv(dev)->real_dev;
>> with
>> struct net_device *ret = dev;
>>
>> This way we'll be able to simplify calls like if (is_vlan_dev(dev)) (or the
>> flag check alternative of this) to just vlan_dev_real_dev(). The old call
>> sites of this function will still work properly.
> 
> I think it'll be a bit harder to understand the callers' code. Now it
> always has the logic "wait, we might be having a vlan here, so lets verify
> if it's really a vlan and if yes - get the real device". However with this
> approach it might look like "we *really* have a vlan here - so lets get the
> real device" (because of the function name - vlan_dev_real_dev) - which is
> wrong. And it doesn't really provide any speed/size improvements - so it's
> kind of useless.
> 
It's not used in fast paths, this suggestion was purely about saving some
lines and indentation levels. And you can still use it the old way if you
prefer it.

> So, unless a better function name can be found (which I can't come up with
> - dev_or_strip_vlan()? dev_devlanitize()? :) ) - I'd stay with the current
>  version. Though, again, I don't have a strong opinion on this.
It's all the same to me as well, I've wasted enough time on these simple
patches so I'd rather see them in (if accepted) than argue about semantics :-)

> 
>>
>> Nik

Re: [PATCH net 1/2] vlan: make vlan_dev_real_dev work over stacked vlans

[David Miller]

From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Sat,  3 Aug 2013 22:07:46 +0200

> From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
> 
> Sometimes we might have stacked vlans on top of each other, and we're
> interested in the first non-vlan real device on the path, so transform
> vlan_dev_real_dev to go over the stacked vlans and extract the first
> non-vlan device.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

Applied.

Re: [PATCH net v2 2/2] net_sched: make dev_trans_start return vlan's real dev trans_start

[David Miller]

From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Sat,  3 Aug 2013 22:07:47 +0200

> From: Nikolay Aleksandrov <Nikolay Aleksandrov nikolay@redhat.com>
> 
> Vlan devices are LLTX and don't update their own trans_start, so if
> dev_trans_start has to be called with a vlan device then 0 or a stale
> value will be returned. Currently the bonding is the only such user, and
> it's needed for proper arp monitoring when the slaves are vlans.
> Fix this by extracting the vlan's real device trans_start.
> 
> Suggested-by: David Miller <davem@davemloft.net>
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

Applied.