[PATCH] vlan: propogate MTU changes

[PATCH] vlan: propogate MTU changes

[Stephen Hemminger]

Propogate MTU changes of underlying device to all related VLAN
devices.
see: https://bugzilla.vyatta.com/show_bug.cgi?id=3742

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
There might be some discussion about whether to preserve MTU changes
on the vlan device if done after startup, but this seems like the best,
most direct fix.

--- a/net/8021q/vlan.c	2008-10-06 17:03:58.000000000 +0200
+++ b/net/8021q/vlan.c	2008-10-06 17:08:33.000000000 +0200
@@ -477,6 +477,17 @@ static int vlan_device_event(struct noti
 
 		break;
 
+	case NETDEV_CHANGEMTU:
+		/* Propogate MTU of underlying device */
+		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
+			vlandev = vlan_group_get_device(grp, i);
+			if (!vlandev)
+				continue;
+
+			vlandev->mtu = dev->mtu;
+		}
+		break;
+
 	case NETDEV_DOWN:
 		/* Put all VLANs for this dev in the down state too.  */
 		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {

Re: [PATCH] vlan: propogate MTU changes

[Patrick McHardy]

Stephen Hemminger wrote:
> Propogate MTU changes of underlying device to all related VLAN
> devices.
> see: https://bugzilla.vyatta.com/show_bug.cgi?id=3742
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> ---
> There might be some discussion about whether to preserve MTU changes
> on the vlan device if done after startup, but this seems like the best,
> most direct fix.

Agreed on both points :) But I think having that dicussion would be
useful since there are some points that make it not so clear cut
in my opinion:

- there are multiple virtual drivers depending on configuration of
   some underlying device and it would be good if this behaviour (MTU)
   was consistent since its about interaction with the remaining stack.

- the stack in fact doesn't require us to reduce the MTU of a VLAN
   device as long as its within the physically possible MTU.

- besides MTU, we have UP/DOWN state - currently VLAN devices go
   down when the lower device goes down, killing all routes, but
   don't go UP again when the lower device does. Even if they would,
   most routes can't be reconstructed. The same is true for at
   least some of the other virtual network devices.

- some more items that are often initially taken from the real
   device, but not synced later on include device and broadcast
   address and some flags (f.i. IFF_NOARP, IFF_BROADCAST).

I talked about especially the UP/DOWN point with Ben and some other
people multiple times, but failed to come up with a one-size-fits-all
behaviour. So maybe we should just add some knob (a device flag or
something similar) that "binds" things like MTU, UP/DOWN state etc.
to the lower device. That would also avoid the potential compatibility
issues.

Re: [PATCH] vlan: propogate MTU changes

[Stephen Hemminger]

On Mon, 06 Oct 2008 18:02:39 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Stephen Hemminger wrote:
> > Propogate MTU changes of underlying device to all related VLAN
> > devices.
> > see: https://bugzilla.vyatta.com/show_bug.cgi?id=3742
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> > ---
> > There might be some discussion about whether to preserve MTU changes
> > on the vlan device if done after startup, but this seems like the best,
> > most direct fix.
> 
> Agreed on both points :) But I think having that dicussion would be
> useful since there are some points that make it not so clear cut
> in my opinion:
> 
> - there are multiple virtual drivers depending on configuration of
>    some underlying device and it would be good if this behaviour (MTU)
>    was consistent since its about interaction with the remaining stack.

Increase is also important, there are two common cases. Reducing MTU
because of PPPoe crap, and increasing it because of Jumbo frames.

> - the stack in fact doesn't require us to reduce the MTU of a VLAN
>    device as long as its within the physically possible MTU.

I think it should call change_mtu so userspace gets notified about both
changes.

> - besides MTU, we have UP/DOWN state - currently VLAN devices go
>    down when the lower device goes down, killing all routes, but
>    don't go UP again when the lower device does. Even if they would,
>    most routes can't be reconstructed. The same is true for at
>    least some of the other virtual network devices.
> 
> - some more items that are often initially taken from the real
>    device, but not synced later on include device and broadcast
>    address and some flags (f.i. IFF_NOARP, IFF_BROADCAST).
> 
> I talked about especially the UP/DOWN point with Ben and some other
> people multiple times, but failed to come up with a one-size-fits-all
> behaviour. So maybe we should just add some knob (a device flag or
> something similar) that "binds" things like MTU, UP/DOWN state etc.
> to the lower device. That would also avoid the potential compatibility
> issues.
> 

[PATCH] vlan: propogate MTU changes (v2)

[Stephen Hemminger]

Propogate MTU changes of underlying device to all VLAN's and
send vlan MTU change out to userspace.

see: https://bugzilla.vyatta.com/show_bug.cgi?id=3742

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/net/8021q/vlan.c	2008-10-06 17:03:58.000000000 +0200
+++ b/net/8021q/vlan.c	2008-10-06 19:55:43.000000000 +0200
@@ -477,6 +477,17 @@ static int vlan_device_event(struct noti
 
 		break;
 
+	case NETDEV_CHANGEMTU:
+		/* Propogate MTU of underlying device */
+		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
+			vlandev = vlan_group_get_device(grp, i);
+			if (!vlandev)
+				continue;
+
+			dev_set_mtu(vlandev, dev->mtu);
+		}
+		break;
+
 	case NETDEV_DOWN:
 		/* Put all VLANs for this dev in the down state too.  */
 		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {

RE: [PATCH] vlan: propogate MTU changes (v2)

[Duyck, Alexander H]

Stephen Hemminger wrote:
> Propogate MTU changes of underlying device to all VLAN's and
> send vlan MTU change out to userspace.
>
> see: https://bugzilla.vyatta.com/show_bug.cgi?id=3742
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
>
> --- a/net/8021q/vlan.c  2008-10-06 17:03:58.000000000 +0200
> +++ b/net/8021q/vlan.c  2008-10-06 19:55:43.000000000 +0200
> @@ -477,6 +477,17 @@ static int vlan_device_event(struct noti
>
>                 break;
>
> +       case NETDEV_CHANGEMTU:
> +               /* Propogate MTU of underlying device */
> +               for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
> +                       vlandev = vlan_group_get_device(grp, i);
> +                       if (!vlandev)
> +                               continue;
> +
> +                       dev_set_mtu(vlandev, dev->mtu);
> +               }
> +               break;
> +
>         case NETDEV_DOWN:
>                 /* Put all VLANs for this dev in the down state too.
>                 */ for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {

It has been a while since I worked with vlan tags, but I was just
curious.  Shouldn't the vlan mtu be set to dev->mtu - VLAN_HLEN?

It seems like if the vlan and the device have the same mtu then
you will always have issues since adding the vlan tag will
increase the overall size and push the packet outside mtu size.

Thanks,

Alex

RE: [PATCH] vlan: propogate MTU changes (v2)

[Ben Hutchings]

On Mon, 2008-10-06 at 11:45 -0700, Duyck, Alexander H wrote:
> Stephen Hemminger wrote:
> > Propogate MTU changes of underlying device to all VLAN's and
> > send vlan MTU change out to userspace.
> >
> > see: https://bugzilla.vyatta.com/show_bug.cgi?id=3742
> >
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> >
> > --- a/net/8021q/vlan.c  2008-10-06 17:03:58.000000000 +0200
> > +++ b/net/8021q/vlan.c  2008-10-06 19:55:43.000000000 +0200
> > @@ -477,6 +477,17 @@ static int vlan_device_event(struct noti
> >
> >                 break;
> >
> > +       case NETDEV_CHANGEMTU:
> > +               /* Propogate MTU of underlying device */
> > +               for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
> > +                       vlandev = vlan_group_get_device(grp, i);
> > +                       if (!vlandev)
> > +                               continue;
> > +
> > +                       dev_set_mtu(vlandev, dev->mtu);
> > +               }
> > +               break;
> > +
> >         case NETDEV_DOWN:
> >                 /* Put all VLANs for this dev in the down state too.
> >                 */ for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
> 
> It has been a while since I worked with vlan tags, but I was just
> curious.  Shouldn't the vlan mtu be set to dev->mtu - VLAN_HLEN?
> 
> It seems like if the vlan and the device have the same mtu then
> you will always have issues since adding the vlan tag will
> increase the overall size and push the packet outside mtu size.

An MTU of 1500 is commonly taken to correspond to the IEEE 802.3 maximum
frame size of 1518 bytes which allows for a VLAN tag, and more generally
maximum frame size is set to MTU + 18.  However, not all drivers agree
with this interpretation.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] vlan: propogate MTU changes

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 614 bytes --]



On Mon, 6 Oct 2008, Stephen Hemminger wrote:

> Propogate MTU changes of underlying device to all related VLAN
> devices.
> see: https://bugzilla.vyatta.com/show_bug.cgi?id=3742

Not sure about this... Some switches (HP ProCurve for example) allow to 
select which vlan accepts jumbo frames and which does not and attach both 
to the same port. It is quite useful as you are able to setup one vlan 
with jumbo frames dedicated for example to servers connected by 1G/10G 
links, and a second vlan with standard 1500 MTU distributed to non 
jumbo-aware switches/workstations.

Best regards,

 				Krzysztof Olędzki

Re: [PATCH] vlan: propogate MTU changes

[Patrick McHardy]

Stephen Hemminger wrote:
> On Mon, 06 Oct 2008 18:02:39 +0200
> Patrick McHardy <kaber@trash.net> wrote:
> 
>> - the stack in fact doesn't require us to reduce the MTU of a VLAN
>>    device as long as its within the physically possible MTU.
> 
> I think it should call change_mtu so userspace gets notified about both
> changes.

Agreed. But the question when to do automatic adjustments remains.

Re: [PATCH] vlan: propogate MTU changes

[Patrick McHardy]

Krzysztof Oledzki wrote:
> 
> 
> On Mon, 6 Oct 2008, Stephen Hemminger wrote:
> 
>> Propogate MTU changes of underlying device to all related VLAN
>> devices.
>> see: https://bugzilla.vyatta.com/show_bug.cgi?id=3742
> 
> Not sure about this... Some switches (HP ProCurve for example) allow to 
> select which vlan accepts jumbo frames and which does not and attach 
> both to the same port. It is quite useful as you are able to setup one 
> vlan with jumbo frames dedicated for example to servers connected by 
> 1G/10G links, and a second vlan with standard 1500 MTU distributed to 
> non jumbo-aware switches/workstations.

Thats why I favour the knob to specify the desired behaviour.
Treating a VLAN device as a seperate entity from the ethernet
device which just has the same hardware-imposed restrictions
seems like a valid view to me.

Re: [PATCH] vlan: propogate MTU changes

[Rick Jones]

Patrick McHardy wrote:
> Stephen Hemminger wrote:
> 
>> On Mon, 06 Oct 2008 18:02:39 +0200
>> Patrick McHardy <kaber@trash.net> wrote:
>>
>>> - the stack in fact doesn't require us to reduce the MTU of a VLAN
>>>    device as long as its within the physically possible MTU.
>>
>>
>> I think it should call change_mtu so userspace gets notified about both
>> changes.
> 
> 
> Agreed. But the question when to do automatic adjustments remains.

A matter of interpretation of the principle of least surprise right? 
Which is less surprising - that a VLAN's MTU drops to match that of the 
physical interface or that some traffic on the VLAN stops when the 
physical interface's MTU drops?

If physical interface MTUs are going to be bouncing around and VLANs get 
their MTUs changed then perhaps a VLAN needs both a desired and actual 
MTU setting.  The VLAN's interface would then be the minimum of the 
desired and actual MTU.  I suppose it isn't too unlike having both an 
administrative (desired) and operational (actual) interface state.

rick jones

Re: [PATCH] vlan: propogate MTU changes (v2)

[Patrick McHardy]

Stephen Hemminger wrote:
> Propogate MTU changes of underlying device to all VLAN's and
> send vlan MTU change out to userspace.
> 
> see: https://bugzilla.vyatta.com/show_bug.cgi?id=3742

This really doesn't answer any of the questions related to this
change, mainly whether this kind of behaviour shouldn't be
user controllable.

Re: [PATCH] vlan: propogate MTU changes

[Patrick McHardy]

Rick Jones wrote:
> Patrick McHardy wrote:
>> Agreed. But the question when to do automatic adjustments remains.
> 
> A matter of interpretation of the principle of least surprise right? 
> Which is less surprising - that a VLAN's MTU drops to match that of the 
> physical interface or that some traffic on the VLAN stops when the 
> physical interface's MTU drops?

The traffic actually shouldn't stop since the MTU isn't enforced by
the lower layers and also usually not by the driver. So I feel unable
to make a policy decision when both views don't seem unreasonable.
Especially given the fact that the "more suprising" behaviour so far
has been our default.

> If physical interface MTUs are going to be bouncing around and VLANs get 
> their MTUs changed then perhaps a VLAN needs both a desired and actual 
> MTU setting.  The VLAN's interface would then be the minimum of the 
> desired and actual MTU.  I suppose it isn't too unlike having both an 
> administrative (desired) and operational (actual) interface state.

Thats assuming that the VLAN device is actually restricted by the
ethernet device settings. I don't know if its always not the case,
but I'm pretty sure it usually isn't. Which means there's no real
need for an operational state wrt. MTUs.

Re: [PATCH] vlan: propogate MTU changes

[Patrick McHardy]

Patrick McHardy wrote:
> Rick Jones wrote:
>> If physical interface MTUs are going to be bouncing around and VLANs 
>> get their MTUs changed then perhaps a VLAN needs both a desired and 
>> actual MTU setting.  The VLAN's interface would then be the minimum of 
>> the desired and actual MTU.  I suppose it isn't too unlike having both 
>> an administrative (desired) and operational (actual) interface state.
> 
> Thats assuming that the VLAN device is actually restricted by the
> ethernet device settings. I don't know if its always not the case,
> but I'm pretty sure it usually isn't. Which means there's no real
> need for an operational state wrt. MTUs.

Actually it would be useful to have this kind of information on the
*ethernet* device to specify the upper limit.

Re: [PATCH] vlan: propogate MTU changes

[Rick Jones]

Patrick McHardy wrote:
> Rick Jones wrote:
> 
>> Patrick McHardy wrote:
>>
>>> Agreed. But the question when to do automatic adjustments remains.
>>
>>
>> A matter of interpretation of the principle of least surprise right? 
>> Which is less surprising - that a VLAN's MTU drops to match that of 
>> the physical interface or that some traffic on the VLAN stops when the 
>> physical interface's MTU drops?
> 
> 
> The traffic actually shouldn't stop since the MTU isn't enforced by
> the lower layers and also usually not by the driver. So I feel unable
> to make a policy decision when both views don't seem unreasonable.
> Especially given the fact that the "more suprising" behaviour so far
> has been our default.

Does changing the MTU on a physical interface not change the size frame 
the NIC itself will be willing to accept?

rick jones

Re: [PATCH] vlan: propogate MTU changes

[Patrick McHardy]

Rick Jones wrote:
> Patrick McHardy wrote:
>> Rick Jones wrote:
>>
>>> Patrick McHardy wrote:
>>>
>>>> Agreed. But the question when to do automatic adjustments remains.
>>>
>>>
>>> A matter of interpretation of the principle of least surprise right? 
>>> Which is less surprising - that a VLAN's MTU drops to match that of 
>>> the physical interface or that some traffic on the VLAN stops when 
>>> the physical interface's MTU drops?
>>
>>
>> The traffic actually shouldn't stop since the MTU isn't enforced by
>> the lower layers and also usually not by the driver. So I feel unable
>> to make a policy decision when both views don't seem unreasonable.
>> Especially given the fact that the "more suprising" behaviour so far
>> has been our default.
> 
> Does changing the MTU on a physical interface not change the size frame 
> the NIC itself will be willing to accept?

IIRC a lot of the simpler ones just use the default eth_setup change_mtu
callback and the ones that have their one (just had a very brief look at
sky2, tg3 and e1000) only seem to use it indirectly for enabling jumbo
frame support and (e1000) memory allocation.

So I guess what we should do in case of the MTU depends on what we can
expect from the majority of hardware. If its just some older drivers
which can be reasonably expected to handle larger frames we should cap
at the maximum of the real device and maybe introduce the "desired
mtu" you suggested. It would be useful if people more familiar with
the drivers and hardware than me could comment on this.

Re: [PATCH] vlan: propogate MTU changes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 07 Oct 2008 01:33:47 +0200

> Rick Jones wrote:
> > Patrick McHardy wrote:
> >> Rick Jones wrote:
> >>
> >>> Patrick McHardy wrote:
> >>>
> >>>> Agreed. But the question when to do automatic adjustments remains.
> >>>
> >>>
> >>> A matter of interpretation of the principle of least surprise right? Which is less surprising - that a VLAN's MTU drops to match that of the physical interface or that some traffic on the VLAN stops when the physical interface's MTU drops?
> >>
> >>
> >> The traffic actually shouldn't stop since the MTU isn't enforced by
> >> the lower layers and also usually not by the driver. So I feel unable
> >> to make a policy decision when both views don't seem unreasonable.
> >> Especially given the fact that the "more suprising" behaviour so far
> >> has been our default.
> > Does changing the MTU on a physical interface not change the size frame the NIC itself will be willing to accept?
> 
> IIRC a lot of the simpler ones just use the default eth_setup change_mtu
> callback and the ones that have their one (just had a very brief look at
> sky2, tg3 and e1000) only seem to use it indirectly for enabling jumbo
> frame support and (e1000) memory allocation.
> 
> So I guess what we should do in case of the MTU depends on what we can
> expect from the majority of hardware. If its just some older drivers
> which can be reasonably expected to handle larger frames we should cap
> at the maximum of the real device and maybe introduce the "desired
> mtu" you suggested. It would be useful if people more familiar with
> the drivers and hardware than me could comment on this.

Since there is no agreement on exactly what we should be doing, I'm
tossing this from my patch queue.

I will say, however, that our current behavior isn't so horrible. :)

Re: [PATCH] vlan: propogate MTU changes

[Patrick McHardy]

Patrick McHardy wrote:
> Rick Jones wrote:
>> Does changing the MTU on a physical interface not change the size 
>> frame the NIC itself will be willing to accept?
> 
> IIRC a lot of the simpler ones just use the default eth_setup change_mtu
> callback and the ones that have their one (just had a very brief look at
> sky2, tg3 and e1000) only seem to use it indirectly for enabling jumbo
> frame support and (e1000) memory allocation.
> 
> So I guess what we should do in case of the MTU depends on what we can
> expect from the majority of hardware. If its just some older drivers
> which can be reasonably expected to handle larger frames we should cap
> at the maximum of the real device and maybe introduce the "desired
> mtu" you suggested. It would be useful if people more familiar with
> the drivers and hardware than me could comment on this.

After looking at more drivers, it seems most new ones actually
enfore the configured MTU by programming the hardware with it
(though I don't know the effects this causes) or using it for
memory allocation.

So I think we should follow your suggestions of "desired/operational
MTU". I'll post a patch shortly.