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

[PATCH net] 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>
---
 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..da936f6 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;
 
+	while (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: [net] net_sched: make dev_trans_start return vlan's real dev trans_start

[Veaceslav Falico]

On Sat, Aug 03, 2013 at 05:07:51PM +0200, nikolay@redhat.com wrote:
...snip...
>+	while (is_vlan_dev(dev))
>+		dev = vlan_dev_real_dev(dev);

While at it - I've checked a few users (mainly network drivers) of
vlan_dev_real_dev(dev) and they all rely on that the return device would be
the *real* device, but not another vlan.

So maybe we should move this while loop to vlan_dev_real_dev() instead?

Re: [net] net_sched: make dev_trans_start return vlan's real dev trans_start

[Nikolay Aleksandrov]

On 08/03/2013 07:07 PM, Veaceslav Falico wrote:
> On Sat, Aug 03, 2013 at 05:07:51PM +0200, nikolay@redhat.com wrote:
> ...snip...
>> +    while (is_vlan_dev(dev))
>> +        dev = vlan_dev_real_dev(dev);
> 
> While at it - I've checked a few users (mainly network drivers) of
> vlan_dev_real_dev(dev) and they all rely on that the return device would be
> the *real* device, but not another vlan.
> 
> So maybe we should move this while loop to vlan_dev_real_dev() instead?
Not really, there're users that rely to get only 1 level of real_dev (e.g.
netxen ip config which expects a configuration like vlan -> bond -> netxen,
and it needs to go from vlan to bond only) which will be broken, there're
also many non-ethernet users which may rely on similar behaviour.
Most of the network drivers don't expect a *real* device, they check if the
returned device is one of their own after using that function.
I'd suggest using a helper that uses vlan_dev_real_dev so we will have both
without breaking anything.

Nik

Re: [net] net_sched: make dev_trans_start return vlan's real dev trans_start

[Nikolay Aleksandrov]

On 08/03/2013 08:13 PM, Nikolay Aleksandrov wrote:
> On 08/03/2013 07:07 PM, Veaceslav Falico wrote:
>> On Sat, Aug 03, 2013 at 05:07:51PM +0200, nikolay@redhat.com wrote:
>> ...snip...
>>> +    while (is_vlan_dev(dev))
>>> +        dev = vlan_dev_real_dev(dev);
>>
>> While at it - I've checked a few users (mainly network drivers) of
>> vlan_dev_real_dev(dev) and they all rely on that the return device would be
>> the *real* device, but not another vlan.
>>
>> So maybe we should move this while loop to vlan_dev_real_dev() instead?
<snip>
> Not really, there're users that rely to get only 1 level of real_dev (e.g.
> netxen ip config which expects a configuration like vlan -> bond -> netxen,
> and it needs to go from vlan to bond only) which will be broken, there're
Scratch this part, I need to get some sleep :-)


> also many non-ethernet users which may rely on similar behaviour.
> Most of the network drivers don't expect a *real* device, they check if the
> returned device is one of their own after using that function.
> I'd suggest using a helper that uses vlan_dev_real_dev so we will have both
> without breaking anything.
> 
> Nik
> --
> 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
> 

Re: [net] net_sched: make dev_trans_start return vlan's real dev trans_start

[Veaceslav Falico]

On Sat, Aug 03, 2013 at 08:19:10PM +0200, Nikolay Aleksandrov wrote:
>On 08/03/2013 08:13 PM, Nikolay Aleksandrov wrote:
>> On 08/03/2013 07:07 PM, Veaceslav Falico wrote:
>>> On Sat, Aug 03, 2013 at 05:07:51PM +0200, nikolay@redhat.com wrote:
>>> ...snip...
>>>> +    while (is_vlan_dev(dev))
>>>> +        dev = vlan_dev_real_dev(dev);
>>>
>>> While at it - I've checked a few users (mainly network drivers) of
>>> vlan_dev_real_dev(dev) and they all rely on that the return device would be
>>> the *real* device, but not another vlan.
>>>
>>> So maybe we should move this while loop to vlan_dev_real_dev() instead?
><snip>
>> Not really, there're users that rely to get only 1 level of real_dev (e.g.
>> netxen ip config which expects a configuration like vlan -> bond -> netxen,
>> and it needs to go from vlan to bond only) which will be broken, there're
>Scratch this part, I need to get some sleep :-)

:)

Yeah, it only modifies the QinQ scenario. I've never used it personally,
though.

Here you go, I think the code is more readable than my english:

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);
  

>
>
>> also many non-ethernet users which may rely on similar behaviour.
>> Most of the network drivers don't expect a *real* device, they check if the
>> returned device is one of their own after using that function.

True, and in case of vlan -> vlan -> dev they'll won't find themselves,
which is bad.

>> I'd suggest using a helper that uses vlan_dev_real_dev so we will have both
>> without breaking anything.

I don't have any preference in that, tbh, I don't know the vlan part that
good :).

>>
>> Nik
>> --
>> 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
>>
>

Re: [net] net_sched: make dev_trans_start return vlan's real dev trans_start

[David Miller]

From: Veaceslav Falico <vfalico@redhat.com>
Date: Sat, 3 Aug 2013 19:07:33 +0200

> On Sat, Aug 03, 2013 at 05:07:51PM +0200, nikolay@redhat.com wrote:
> ...snip...
>>+	while (is_vlan_dev(dev))
>>+		dev = vlan_dev_real_dev(dev);
> 
> While at it - I've checked a few users (mainly network drivers) of
> vlan_dev_real_dev(dev) and they all rely on that the return device
> would be the *real* device, but not another vlan.

Did you find any cases that want the device under the VLAN,
whether it is a non-vlan device or not?

> So maybe we should move this while loop to vlan_dev_real_dev()
> instead?

Perhaps.  As per above, we may also need the one-level demux
helper too, something like "vlan_dev_slave(dev)".

Re: [net] net_sched: make dev_trans_start return vlan's real dev trans_start

[Veaceslav Falico]

On Sat, Aug 03, 2013 at 11:52:28AM -0700, David Miller wrote:
>From: Veaceslav Falico <vfalico@redhat.com>
>Date: Sat, 3 Aug 2013 19:07:33 +0200
>
>> On Sat, Aug 03, 2013 at 05:07:51PM +0200, nikolay@redhat.com wrote:
>> ...snip...
>>>+	while (is_vlan_dev(dev))
>>>+		dev = vlan_dev_real_dev(dev);
>>
>> While at it - I've checked a few users (mainly network drivers) of
>> vlan_dev_real_dev(dev) and they all rely on that the return device
>> would be the *real* device, but not another vlan.
>
>Did you find any cases that want the device under the VLAN,
>whether it is a non-vlan device or not?

Not really. All of the cases seem to explicitly call it to get a non-vlan
device, and not the purely 'underlying' one.

Another point is that they won't work properly with QinQ... cause the
majority of callers are searching for their own device. Or bonding, as in
netxen that Nik mentioned. And they will find yet another vlan device.

So either QinQ isn't really that used or I'm missing something...

>
>> So maybe we should move this while loop to vlan_dev_real_dev()
>> instead?
>
>Perhaps.  As per above, we may also need the one-level demux
>helper too, something like "vlan_dev_slave(dev)".

I haven't found any usage for it, tbh. Only the vlan code itself might
benefit from it, but it already uses the vlan_dev_priv(dev)->real_dev, and
not the vlan_dev_real_dev(), which is used by drivers.

Re: [net] net_sched: make dev_trans_start return vlan's real dev trans_start

[Nikolay Aleksandrov]

On 08/03/2013 09:09 PM, Veaceslav Falico wrote:
> On Sat, Aug 03, 2013 at 11:52:28AM -0700, David Miller wrote:
>> From: Veaceslav Falico <vfalico@redhat.com>
>> Date: Sat, 3 Aug 2013 19:07:33 +0200
>>
>>> On Sat, Aug 03, 2013 at 05:07:51PM +0200, nikolay@redhat.com wrote:
>>> ...snip...
>>>> +    while (is_vlan_dev(dev))
>>>> +        dev = vlan_dev_real_dev(dev);
>>>
>>> While at it - I've checked a few users (mainly network drivers) of
>>> vlan_dev_real_dev(dev) and they all rely on that the return device
>>> would be the *real* device, but not another vlan.
>>
>> Did you find any cases that want the device under the VLAN,
>> whether it is a non-vlan device or not?
> 
> Not really. All of the cases seem to explicitly call it to get a non-vlan
> device, and not the purely 'underlying' one.
> 
> Another point is that they won't work properly with QinQ... cause the
> majority of callers are searching for their own device. Or bonding, as in
> netxen that Nik mentioned. And they will find yet another vlan device.
> 
> So either QinQ isn't really that used or I'm missing something...
> 
>>
>>> So maybe we should move this while loop to vlan_dev_real_dev()
>>> instead?
>>
>> Perhaps.  As per above, we may also need the one-level demux
>> helper too, something like "vlan_dev_slave(dev)".
> 
> I haven't found any usage for it, tbh. Only the vlan code itself might
> benefit from it, but it already uses the vlan_dev_priv(dev)->real_dev, and
> not the vlan_dev_real_dev(), which is used by drivers.
I agree with this. I also can't find users of the one-level variant, so I
think it'd be safe to go ahead and do it.
Just one question in case that we all agree -
Dave should I queue the patches (vlan change & net_sched's dev_trans_start)
for net-next or net would be fine ?

Thanks,
 Nik

Re: [net] net_sched: make dev_trans_start return vlan's real dev trans_start

[David Miller]

From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Sat, 03 Aug 2013 21:35:20 +0200

> Dave should I queue the patches (vlan change & net_sched's dev_trans_start)
> for net-next or net would be fine ?

I think these are bugs to 'net' is better.

Re: [net] net_sched: make dev_trans_start return vlan's real dev trans_start

[David Miller]

From: Veaceslav Falico <vfalico@redhat.com>
Date: Sat, 3 Aug 2013 21:09:18 +0200

> I haven't found any usage for it, tbh. Only the vlan code itself
> might benefit from it, but it already uses the
> vlan_dev_priv(dev)->real_dev, and not the vlan_dev_real_dev(), which
> is used by drivers.

Agreed.