Question about VLAN + checksum offloading

Question about VLAN + checksum offloading

[Ichiro Suzuki]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

Hi,

  Looking in oprofile log, I observed
	csum_partial_copy_generic()
is invoked for a 802.1Q VLAN device created on e1000,
This suggests checksum offloading is not in effect.
The offloading works fine when e1000 is used directly.
Attached one liner patch fixes this. But, it seems
too obvious to be a correct answer.

  My questions are,
o Is it right to expect checksum offloading should work
 on VLAN devices?
o If so, is there any mechanism to propagate
 real_dev->features flags in vlan.c?
o If such mechanism doesn't exist, is my patch reasonable?
.

  The machine arch is x86_64.  I got similar results
on an ancient 2.6.9 kernel and on the latest 2.6.25.

Thanks in advance,
----------------------------------------------
Ichiro Suzuki       <isuzuki@miraclelinux.com>
Miracle Linux Corp., Advanced Technology Group
----------------------------------------------


[-- Attachment #2: linux-2.6.25_vlan.patch --]
[-- Type: text/x-patch, Size: 476 bytes --]

--- linux-2.6.25.orig/net/8021q/vlan.c	2008-04-17 11:49:44.000000000 +0900
+++ linux-2.6.25/net/8021q/vlan.c	2008-05-16 15:03:03.000000000 +0900
@@ -333,6 +333,9 @@ static int register_vlan_device(struct n
 	 */
 	new_dev->mtu = real_dev->mtu;
 
+	/* features setting */
+	new_dev->features = real_dev->features;
+
 	vlan_dev_info(new_dev)->vlan_id = VLAN_ID; /* 1 through VLAN_VID_MASK */
 	vlan_dev_info(new_dev)->real_dev = real_dev;
 	vlan_dev_info(new_dev)->dent = NULL;

RE: Question about VLAN + checksum offloading

[Waskiewicz Jr, Peter P]

> Hi,
> 
>   Looking in oprofile log, I observed
> 	csum_partial_copy_generic()
> is invoked for a 802.1Q VLAN device created on e1000, This 
> suggests checksum offloading is not in effect.
> The offloading works fine when e1000 is used directly.
> Attached one liner patch fixes this. But, it seems too 
> obvious to be a correct answer.

You would think so.

>   My questions are,
> o Is it right to expect checksum offloading should work  on 
> VLAN devices?

It should, but perhaps not.  Another feature is TSO + VLAN.  This was
something I proposed to patch earlier, in the same fashion you have
here.  However, some hardware out there may not be able to handle both
hardware offloads at the same time.  I'm not aware of which hardware
that would be, but it was hinted to me that such hardware exists.

> o If so, is there any mechanism to propagate  
> real_dev->features flags in vlan.c?

There isn't an explicit way.  I had written patches into e1000, igb,
e1000e, and ixgbe to propogate the VLAN flags within the driver when the
VLAN device was created.  The trick though is if you remove a feature
flag with ethtool, say checksum offload, on your main device, you
probably should turn it off on your VLAN devices.  Patrick McHardy
pointed me at netdev_feature_change() to use within the driver.  I'll
admit I haven't had the time to fix my drivers to use this call, but it
certainly looks like the way to go.  Please see the (middle) of the
thread here: http://marc.info/?l=linux-netdev&m=120878809806631&w=2

> o If such mechanism doesn't exist, is my patch reasonable?

I would say yes, halfway.  The issue is you probably want to remove the
feature flag from the VLAN device if you removed the flag from the
parent device as well.

Cheers,

-PJ Waskiewicz
<peter.p.waskiewicz.jr@intel.com>

Re: Question about VLAN + checksum offloading

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>> o If so, is there any mechanism to propagate  
>> real_dev->features flags in vlan.c?
> 
> There isn't an explicit way.  I had written patches into e1000, igb,
> e1000e, and ixgbe to propogate the VLAN flags within the driver when the
> VLAN device was created.  The trick though is if you remove a feature
> flag with ethtool, say checksum offload, on your main device, you
> probably should turn it off on your VLAN devices.  Patrick McHardy
> pointed me at netdev_feature_change() to use within the driver.  I'll
> admit I haven't had the time to fix my drivers to use this call, but it
> certainly looks like the way to go.  Please see the (middle) of the
> thread here: http://marc.info/?l=linux-netdev&m=120878809806631&w=2
> 
>> o If such mechanism doesn't exist, is my patch reasonable?
> 
> I would say yes, halfway.  The issue is you probably want to remove the
> feature flag from the VLAN device if you removed the flag from the
> parent device as well.


Yes, it should use the same mechanism as suggested for the
VLAN accel feature. And it should be limited to features
that are known to work, not just blindly copy everything.

I will be catching up with the VLAN patches posted recently
sometime next week. I guess I can then also add a patch for
feature propagation myself if you don't beat me to it :)

Re: Question about VLAN + checksum offloading

[Patrick McHardy]

Patrick McHardy wrote:
> Waskiewicz Jr, Peter P wrote:
>>> o If so, is there any mechanism to propagate  real_dev->features 
>>> flags in vlan.c?
>>
>> There isn't an explicit way.  I had written patches into e1000, igb,
>> e1000e, and ixgbe to propogate the VLAN flags within the driver when the
>> VLAN device was created.  The trick though is if you remove a feature
>> flag with ethtool, say checksum offload, on your main device, you
>> probably should turn it off on your VLAN devices.  Patrick McHardy
>> pointed me at netdev_feature_change() to use within the driver.  I'll
>> admit I haven't had the time to fix my drivers to use this call, but it
>> certainly looks like the way to go.  Please see the (middle) of the
>> thread here: http://marc.info/?l=linux-netdev&m=120878809806631&w=2
>>
>>> o If such mechanism doesn't exist, is my patch reasonable?
>>
>> I would say yes, halfway.  The issue is you probably want to remove the
>> feature flag from the VLAN device if you removed the flag from the
>> parent device as well.
> 
> 
> Yes, it should use the same mechanism as suggested for the
> VLAN accel feature. And it should be limited to features

I meant TSO, sorry.

> that are known to work, not just blindly copy everything.
> 
> I will be catching up with the VLAN patches posted recently
> sometime next week. I guess I can then also add a patch for
> feature propagation myself if you don't beat me to it :)

Re: Question about VLAN + checksum offloading

[Ichiro Suzuki]

Thank you, PJ and Patrick.
I'll revisit the code after VLAN patches are posted.

----------------------------------------------
Ichiro Suzuki       <isuzuki@miraclelinux.com>
Miracle Linux Corp., Advanced Technology Group
----------------------------------------------

On Mon, 2008-05-19 at 19:43 +0200, Patrick McHardy wrote:
> Waskiewicz Jr, Peter P wrote:
> >> o If so, is there any mechanism to propagate  
> >> real_dev->features flags in vlan.c?
> > 
> > There isn't an explicit way.  I had written patches into e1000, igb,
> > e1000e, and ixgbe to propogate the VLAN flags within the driver when the
> > VLAN device was created.  The trick though is if you remove a feature
> > flag with ethtool, say checksum offload, on your main device, you
> > probably should turn it off on your VLAN devices.  Patrick McHardy
> > pointed me at netdev_feature_change() to use within the driver.  I'll
> > admit I haven't had the time to fix my drivers to use this call, but it
> > certainly looks like the way to go.  Please see the (middle) of the
> > thread here: http://marc.info/?l=linux-netdev&m=120878809806631&w=2
> > 
> >> o If such mechanism doesn't exist, is my patch reasonable?
> > 
> > I would say yes, halfway.  The issue is you probably want to remove the
> > feature flag from the VLAN device if you removed the flag from the
> > parent device as well.
> 
> 
> Yes, it should use the same mechanism as suggested for the
> VLAN accel feature. And it should be limited to features
> that are known to work, not just blindly copy everything.
> 
> I will be catching up with the VLAN patches posted recently
> sometime next week. I guess I can then also add a patch for
> feature propagation myself if you don't beat me to it :)
> 
>