[RFC, VLAN]: Propagate selected feature bits to VLAN devices

[RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

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

This patch implements the TSO feature propagation we discussed.
Only compile-tested so far, testing and comments welcome.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3030 bytes --]

commit b01a0b6b4a0463fe896e166d0782f0daca8c69ff
Author: Patrick McHardy <kaber@trash.net>
Date:   Tue May 20 16:34:51 2008 +0200

    [VLAN]: Propagate selected feature bits to VLAN devices
    
    Propagate feature bits from the NETDEV_FEAT_CHANGE notifier. For now
    only TSO is propagated for devices that announce their ability to
    support TSO in combination with VLAN accel by setting the NETIF_F_VLAN_TSO
    flag.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b11e6e1..3be4559 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -514,10 +514,11 @@ struct net_device
 #define NETIF_F_NETNS_LOCAL	8192	/* Does not change network namespaces */
 #define NETIF_F_MULTI_QUEUE	16384	/* Has multiple TX/RX queues */
 #define NETIF_F_LRO		32768	/* large receive offload */
+#define NETIF_F_VLAN_TSO	65536	/* Supports TSO for VLANs */
 
 	/* Segmentation offload features */
-#define NETIF_F_GSO_SHIFT	16
-#define NETIF_F_GSO_MASK	0xffff0000
+#define NETIF_F_GSO_SHIFT	20
+#define NETIF_F_GSO_MASK	0xfff00000
 #define NETIF_F_TSO		(SKB_GSO_TCPV4 << NETIF_F_GSO_SHIFT)
 #define NETIF_F_UFO		(SKB_GSO_UDP << NETIF_F_GSO_SHIFT)
 #define NETIF_F_GSO_ROBUST	(SKB_GSO_DODGY << NETIF_F_GSO_SHIFT)
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 2a739ad..1f172f7 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -382,6 +382,19 @@ static void vlan_sync_address(struct net_device *dev,
 	memcpy(vlan->real_dev_addr, dev->dev_addr, ETH_ALEN);
 }
 
+static void vlan_transfer_features(struct net_device *dev,
+				   struct net_device *vlandev)
+{
+	unsigned long old_features = vlandev->features;
+
+	if (dev->features & NETIF_F_VLAN_TSO)
+		vlandev->features |= dev->features &
+				     (NETIF_F_TSO | NETIF_F_TSO6);
+
+	if (old_features != vlandev->features)
+		netdev_features_change(vlandev);
+}
+
 static void __vlan_device_event(struct net_device *dev, unsigned long event)
 {
 	switch (event) {
@@ -450,6 +463,18 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		}
 		break;
 
+	case NETDEV_FEAT_CHANGE:
+		/* Propagate device features to underlying device */
+		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
+			vlandev = vlan_group_get_device(grp, i);
+			if (!vlandev)
+				continue;
+
+			vlan_transfer_features(dev, vlandev);
+		}
+
+		break;
+
 	case NETDEV_DOWN:
 		/* Put all VLANs for this dev in the down state too.  */
 		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index c961f08..3705d5a 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -662,6 +662,7 @@ static int vlan_dev_init(struct net_device *dev)
 	dev->state  = (real_dev->state & ((1<<__LINK_STATE_NOCARRIER) |
 					  (1<<__LINK_STATE_DORMANT))) |
 		      (1<<__LINK_STATE_PRESENT);
+	dev->features = real_dev->features & (NETIF_F_TSO | NETIF_F_TSO6);
 
 	/* ipv6 shared card related stuff */
 	dev->dev_id = real_dev->dev_id;

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

Patrick McHardy wrote:
> This patch implements the TSO feature propagation we discussed.
> Only compile-tested so far, testing and comments welcome.

Oops, wrong Peter, please ignore :)

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Ben Hutchings]

Patrick McHardy wrote:
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index b11e6e1..3be4559 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -514,10 +514,11 @@ struct net_device
>  #define NETIF_F_NETNS_LOCAL	8192	/* Does not change network namespaces */
>  #define NETIF_F_MULTI_QUEUE	16384	/* Has multiple TX/RX queues */
>  #define NETIF_F_LRO		32768	/* large receive offload */
> +#define NETIF_F_VLAN_TSO	65536	/* Supports TSO for VLANs */
> 
>  	/* Segmentation offload features */
> -#define NETIF_F_GSO_SHIFT	16
> -#define NETIF_F_GSO_MASK	0xffff0000
> +#define NETIF_F_GSO_SHIFT	20
> +#define NETIF_F_GSO_MASK	0xfff00000

I really don't think it's a good idea to move around existing flags.  How
about stealing some of the unused high-order bits of NETIF_F_GSO_MASK
instead?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

Ben Hutchings wrote:
> Patrick McHardy wrote:
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index b11e6e1..3be4559 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -514,10 +514,11 @@ struct net_device
>>  #define NETIF_F_NETNS_LOCAL	8192	/* Does not change network namespaces */
>>  #define NETIF_F_MULTI_QUEUE	16384	/* Has multiple TX/RX queues */
>>  #define NETIF_F_LRO		32768	/* large receive offload */
>> +#define NETIF_F_VLAN_TSO	65536	/* Supports TSO for VLANs */
>>
>>  	/* Segmentation offload features */
>> -#define NETIF_F_GSO_SHIFT	16
>> -#define NETIF_F_GSO_MASK	0xffff0000
>> +#define NETIF_F_GSO_SHIFT	20
>> +#define NETIF_F_GSO_MASK	0xfff00000
> 
> I really don't think it's a good idea to move around existing flags.  How
> about stealing some of the unused high-order bits of NETIF_F_GSO_MASK
> instead?


Mhh it doesn't really belong there. Whats the problem with
moving these bits? They are only used internally (and use
up too much space anyway).

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Ben Greear]

Patrick McHardy wrote:
> Ben Hutchings wrote:
>> Patrick McHardy wrote:
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index b11e6e1..3be4559 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -514,10 +514,11 @@ struct net_device
>>>  #define NETIF_F_NETNS_LOCAL    8192    /* Does not change network 
>>> namespaces */
>>>  #define NETIF_F_MULTI_QUEUE    16384    /* Has multiple TX/RX 
>>> queues */
>>>  #define NETIF_F_LRO        32768    /* large receive offload */
>>> +#define NETIF_F_VLAN_TSO    65536    /* Supports TSO for VLANs */
>>>
>>>      /* Segmentation offload features */
>>> -#define NETIF_F_GSO_SHIFT    16
>>> -#define NETIF_F_GSO_MASK    0xffff0000
>>> +#define NETIF_F_GSO_SHIFT    20
>>> +#define NETIF_F_GSO_MASK    0xfff00000
>>
>> I really don't think it's a good idea to move around existing flags.  
>> How
>> about stealing some of the unused high-order bits of NETIF_F_GSO_MASK
>> instead?
>
>
> Mhh it doesn't really belong there. Whats the problem with
> moving these bits? They are only used internally (and use
> up too much space anyway).
I've been running with it shifted to 24 on 2.6.25 with no problems.   If 
no one else objects, I'd prefer it shifted all the way
to 24 to make room for more flags w/out additional changes in the 
shiftage later...

Thanks,
Ben

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


-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Ben Hutchings]

Ben Greear wrote:
> Patrick McHardy wrote:
> >Ben Hutchings wrote:
> >>Patrick McHardy wrote:
> >>>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >>>index b11e6e1..3be4559 100644
> >>>--- a/include/linux/netdevice.h
> >>>+++ b/include/linux/netdevice.h
> >>>@@ -514,10 +514,11 @@ struct net_device
> >>> #define NETIF_F_NETNS_LOCAL    8192    /* Does not change network 
> >>>namespaces */
> >>> #define NETIF_F_MULTI_QUEUE    16384    /* Has multiple TX/RX 
> >>>queues */
> >>> #define NETIF_F_LRO        32768    /* large receive offload */
> >>>+#define NETIF_F_VLAN_TSO    65536    /* Supports TSO for VLANs */
> >>>
> >>>     /* Segmentation offload features */
> >>>-#define NETIF_F_GSO_SHIFT    16
> >>>-#define NETIF_F_GSO_MASK    0xffff0000
> >>>+#define NETIF_F_GSO_SHIFT    20
> >>>+#define NETIF_F_GSO_MASK    0xfff00000
> >>
> >>I really don't think it's a good idea to move around existing flags.  
> >>How
> >>about stealing some of the unused high-order bits of NETIF_F_GSO_MASK
> >>instead?
> >
> >
> >Mhh it doesn't really belong there. Whats the problem with
> >moving these bits? They are only used internally (and use
> >up too much space anyway).
> I've been running with it shifted to 24 on 2.6.25 with no problems.   If 
> no one else objects, I'd prefer it shifted all the way
> to 24 to make room for more flags w/out additional changes in the 
> shiftage later...

I would certainly be happy to see NETIF_F_GSO_MASK narrowed.  It's just a
question of whether the low-order or high-order bits are removed.  My
instinct is not to change the existing assigned flags if it's not really
necessary.  They are exposed through /sys/class/net/ even if the flag
names aren't part of the user-land headers.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

Ben Hutchings wrote:
> Ben Greear wrote:
>> Patrick McHardy wrote:
>>> Ben Hutchings wrote:
>>>> I really don't think it's a good idea to move around existing flags.  
>>>> How
>>>> about stealing some of the unused high-order bits of NETIF_F_GSO_MASK
>>>> instead?
>>>
>>> Mhh it doesn't really belong there. Whats the problem with
>>> moving these bits? They are only used internally (and use
>>> up too much space anyway).
>> I've been running with it shifted to 24 on 2.6.25 with no problems.   If 
>> no one else objects, I'd prefer it shifted all the way
>> to 24 to make room for more flags w/out additional changes in the 
>> shiftage later...
> 
> I would certainly be happy to see NETIF_F_GSO_MASK narrowed.  It's just a
> question of whether the low-order or high-order bits are removed.  My
> instinct is not to change the existing assigned flags if it's not really
> necessary.  They are exposed through /sys/class/net/ even if the flag
> names aren't part of the user-land headers.


I don't really care which bits exactly are used, so sure, I'll
update it before sending a final patch. But frankly, these bits
shouldn't have been exported through sysfs in the first place.

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Ben Greear]

Ben Hutchings wrote:
> I would certainly be happy to see NETIF_F_GSO_MASK narrowed.  It's just a
> question of whether the low-order or high-order bits are removed.  My
> instinct is not to change the existing assigned flags if it's not really
> necessary.  They are exposed through /sys/class/net/ even if the flag
> names aren't part of the user-land headers.
>
> Ben.
>   
I guess it doesn't matter so much to me either way..so long as it is 
narrowed one
way or another, and preferably narrowed enough to allow plenty of new flags
down the road.

Thanks,
Ben



-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

RE: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Waskiewicz Jr, Peter P]

> I guess it doesn't matter so much to me either way..so long 
> as it is narrowed one way or another, and preferably narrowed 
> enough to allow plenty of new flags down the road.

In addition to TSO, we also want TCP checksum offload, so we'll need at
least one more flag.  So whatever is done to shrink the GSO bits, we'll
need room for two new flags.

The patch looks good after a visual inspection.  I'll send test results
later today.

Cheers,
-PJ Waskiewicz

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>> I guess it doesn't matter so much to me either way..so long 
>> as it is narrowed one way or another, and preferably narrowed 
>> enough to allow plenty of new flags down the road.
> 
> In addition to TSO, we also want TCP checksum offload, so we'll need at
> least one more flag.  So whatever is done to shrink the GSO bits, we'll
> need room for two new flags.

You're right of course, seems I wasn't thinking :)

> 
> The patch looks good after a visual inspection.  I'll send test results
> later today.

Thanks.

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Ben Hutchings]

Patrick McHardy wrote:
[...]
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -382,6 +382,19 @@ static void vlan_sync_address(struct net_device *dev,
>  	memcpy(vlan->real_dev_addr, dev->dev_addr, ETH_ALEN);
>  }
>  
> +static void vlan_transfer_features(struct net_device *dev,
> +				   struct net_device *vlandev)
> +{
> +	unsigned long old_features = vlandev->features;
> +
> +	if (dev->features & NETIF_F_VLAN_TSO)
> +		vlandev->features |= dev->features &
> +				     (NETIF_F_TSO | NETIF_F_TSO6);

Shouldn't this mask the flags out of vlandev->features first?

> +	if (old_features != vlandev->features)
> +		netdev_features_change(vlandev);
> +}
> +
>  static void __vlan_device_event(struct net_device *dev, unsigned long event)
>  {
>  	switch (event) {
[...]
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index c961f08..3705d5a 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -662,6 +662,7 @@ static int vlan_dev_init(struct net_device *dev)
>  	dev->state  = (real_dev->state & ((1<<__LINK_STATE_NOCARRIER) |
>  					  (1<<__LINK_STATE_DORMANT))) |
>  		      (1<<__LINK_STATE_PRESENT);
> +	dev->features = real_dev->features & (NETIF_F_TSO | NETIF_F_TSO6);
>  
>  	/* ipv6 shared card related stuff */
>  	dev->dev_id = real_dev->dev_id;

This should also test the NETIF_F_VLAN_TSO flag and should copy the
NETIF_F_ALL_CSUM and NETIF_F_SG flags since TSO requires those (see
register_netdevice() feature fix-up).  Peter pointed out that TX checksum
offload for VLANs might be available even without TSO; the same goes for
scatter-gather but I don't know whether it's worth adding a flag for that.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

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

Ben Hutchings wrote:
> Patrick McHardy wrote:
>> +	if (dev->features & NETIF_F_VLAN_TSO)
>> +		vlandev->features |= dev->features &
>> +				     (NETIF_F_TSO | NETIF_F_TSO6);
>>     
>
> Shouldn't this mask the flags out of vlandev->features first?
>   
Yes, thanks.
>> +	dev->features = real_dev->features & (NETIF_F_TSO | NETIF_F_TSO6);
>>     
> This should also test the NETIF_F_VLAN_TSO flag and should copy the
> NETIF_F_ALL_CSUM and NETIF_F_SG flags since TSO requires those (see
> register_netdevice() feature fix-up).  Peter pointed out that TX checksum
> offload for VLANs might be available even without TSO; the same goes for
> scatter-gather but I don't know whether it's worth adding a flag for that.
>   

Thanks, I added NETIF_F_SG to the VLAN TSO flags and added a
NETIF_F_VLAN_CSUM flag for TX checksums. I didn't change
GSO_SHIFT so far.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3896 bytes --]

commit fd467a0422c9c8ddd05c53c4f91d4af71c8fb9ff
Author: Patrick McHardy <kaber@trash.net>
Date:   Tue May 20 20:31:40 2008 +0200

    [VLAN]: Propagate selected feature bits to VLAN devices
    
    Propagate feature bits from the NETDEV_FEAT_CHANGE notifier. For now
    only TSO is propagated for devices that announce their ability to
    support TSO in combination with VLAN accel by setting the NETIF_F_VLAN_TSO
    flag.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b11e6e1..2b02664 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -514,10 +514,12 @@ struct net_device
 #define NETIF_F_NETNS_LOCAL	8192	/* Does not change network namespaces */
 #define NETIF_F_MULTI_QUEUE	16384	/* Has multiple TX/RX queues */
 #define NETIF_F_LRO		32768	/* large receive offload */
+#define NETIF_F_VLAN_TSO	65536	/* Supports TSO for VLANs */
+#define NETIF_F_VLAN_CSUM	131072	/* Supports TX checksumming for VLANs */
 
 	/* Segmentation offload features */
-#define NETIF_F_GSO_SHIFT	16
-#define NETIF_F_GSO_MASK	0xffff0000
+#define NETIF_F_GSO_SHIFT	20
+#define NETIF_F_GSO_MASK	0xfff00000
 #define NETIF_F_TSO		(SKB_GSO_TCPV4 << NETIF_F_GSO_SHIFT)
 #define NETIF_F_UFO		(SKB_GSO_UDP << NETIF_F_GSO_SHIFT)
 #define NETIF_F_GSO_ROBUST	(SKB_GSO_DODGY << NETIF_F_GSO_SHIFT)
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 2a739ad..20fb6fd 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -382,6 +382,24 @@ static void vlan_sync_address(struct net_device *dev,
 	memcpy(vlan->real_dev_addr, dev->dev_addr, ETH_ALEN);
 }
 
+static void vlan_transfer_features(struct net_device *dev,
+				   struct net_device *vlandev)
+{
+	unsigned long old_features = vlandev->features;
+
+	if (dev->features & NETIF_F_VLAN_TSO) {
+		vlandev->features &= ~VLAN_TSO_FEATURES;
+		vlandev->features |= dev->features & VLAN_TSO_FEATURES;
+	}
+	if (dev->features & NETIF_F_VLAN_CSUM) {
+		vlandev->features &= ~NETIF_F_ALL_CSUM;
+		vlandev->features |= dev->features & NETIF_F_ALL_CSUM;
+	}
+
+	if (old_features != vlandev->features)
+		netdev_features_change(vlandev);
+}
+
 static void __vlan_device_event(struct net_device *dev, unsigned long event)
 {
 	switch (event) {
@@ -450,6 +468,18 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		}
 		break;
 
+	case NETDEV_FEAT_CHANGE:
+		/* Propagate device features to underlying device */
+		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
+			vlandev = vlan_group_get_device(grp, i);
+			if (!vlandev)
+				continue;
+
+			vlan_transfer_features(dev, vlandev);
+		}
+
+		break;
+
 	case NETDEV_DOWN:
 		/* Put all VLANs for this dev in the down state too.  */
 		for (i = 0; i < VLAN_GROUP_ARRAY_LEN; i++) {
diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
index 5229a72..7962569 100644
--- a/net/8021q/vlan.h
+++ b/net/8021q/vlan.h
@@ -7,6 +7,8 @@
 #define VLAN_GRP_HASH_SIZE	(1 << VLAN_GRP_HASH_SHIFT)
 #define VLAN_GRP_HASH_MASK	(VLAN_GRP_HASH_SIZE - 1)
 
+#define VLAN_TSO_FEATURES	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_SG)
+
 /*  Find a VLAN device by the MAC address of its Ethernet device, and
  *  it's VLAN ID.  The default configuration is to have VLAN's scope
  *  to be box-wide, so the MAC will be ignored.  The mac will only be
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index c961f08..b1cfbaa 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -663,6 +663,11 @@ static int vlan_dev_init(struct net_device *dev)
 					  (1<<__LINK_STATE_DORMANT))) |
 		      (1<<__LINK_STATE_PRESENT);
 
+	if (real_dev->features & NETIF_F_VLAN_TSO)
+		dev->features |= real_dev->features & VLAN_TSO_FEATURES;
+	if (real_dev->features & NETIF_F_VLAN_CSUM)
+		dev->features |= real_dev->features & NETIF_F_ALL_CSUM;
+
 	/* ipv6 shared card related stuff */
 	dev->dev_id = real_dev->dev_id;
 

RE: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Waskiewicz Jr, Peter P]

> > Shouldn't this mask the flags out of vlandev->features first?
> >   
> Yes, thanks.
> >> +	dev->features = real_dev->features & (NETIF_F_TSO | 
> NETIF_F_TSO6);
> >>     
> > This should also test the NETIF_F_VLAN_TSO flag and should copy the 
> > NETIF_F_ALL_CSUM and NETIF_F_SG flags since TSO requires those (see
> > register_netdevice() feature fix-up).  Peter pointed out that TX 
> > checksum offload for VLANs might be available even without TSO; the 
> > same goes for scatter-gather but I don't know whether it's 
> worth adding a flag for that.
> >   
> 
> Thanks, I added NETIF_F_SG to the VLAN TSO flags and added a 
> NETIF_F_VLAN_CSUM flag for TX checksums. I didn't change 
> GSO_SHIFT so far.

I've tested your latest patch with an in-house modified ixgbe driver,
both with CSUM and TSO.  This patch works great for adding and removing
the feature flags on demand.  Thanks for putting this together Patrick.

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

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>>> Shouldn't this mask the flags out of vlandev->features first?
>>>   
>> Yes, thanks.
>>>> +	dev->features = real_dev->features & (NETIF_F_TSO | 
>> NETIF_F_TSO6);
>>>>     
>>> This should also test the NETIF_F_VLAN_TSO flag and should copy the 
>>> NETIF_F_ALL_CSUM and NETIF_F_SG flags since TSO requires those (see
>>> register_netdevice() feature fix-up).  Peter pointed out that TX 
>>> checksum offload for VLANs might be available even without TSO; the 
>>> same goes for scatter-gather but I don't know whether it's 
>> worth adding a flag for that.
>>>   
>> Thanks, I added NETIF_F_SG to the VLAN TSO flags and added a 
>> NETIF_F_VLAN_CSUM flag for TX checksums. I didn't change 
>> GSO_SHIFT so far.
> 
> I've tested your latest patch with an in-house modified ixgbe driver,
> both with CSUM and TSO.  This patch works great for adding and removing
> the feature flags on demand.  Thanks for putting this together Patrick.
> 
> -PJ Waskiewicz
> <peter.p.waskiewicz.jr@intel.com>

Great, I didn't notice your email before I saw Dave already
applied it :) Thanks Peter.

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Tue, 20 May 2008 20:31:48 +0200

> commit fd467a0422c9c8ddd05c53c4f91d4af71c8fb9ff
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Tue May 20 20:31:40 2008 +0200
> 
>     [VLAN]: Propagate selected feature bits to VLAN devices
>     
>     Propagate feature bits from the NETDEV_FEAT_CHANGE notifier. For now
>     only TSO is propagated for devices that announce their ability to
>     support TSO in combination with VLAN accel by setting the NETIF_F_VLAN_TSO
>     flag.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied, thanks Patrick!

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Tue, 20 May 2008 20:31:48 +0200
> 
>> commit fd467a0422c9c8ddd05c53c4f91d4af71c8fb9ff
>> Author: Patrick McHardy <kaber@trash.net>
>> Date:   Tue May 20 20:31:40 2008 +0200
>>
>>     [VLAN]: Propagate selected feature bits to VLAN devices
>>     
>>     Propagate feature bits from the NETDEV_FEAT_CHANGE notifier. For now
>>     only TSO is propagated for devices that announce their ability to
>>     support TSO in combination with VLAN accel by setting the NETIF_F_VLAN_TSO
>>     flag.
>>     
>>     Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> Applied, thanks Patrick!


Oops, actually this one was only meant for discussion at this
time. I guess I should give it some testing tommorrow :)

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Herbert Xu]

Patrick McHardy <kaber@trash.net> wrote:
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index b11e6e1..3be4559 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -514,10 +514,11 @@ struct net_device
> #define NETIF_F_NETNS_LOCAL    8192    /* Does not change network namespaces */
> #define NETIF_F_MULTI_QUEUE    16384   /* Has multiple TX/RX queues */
> #define NETIF_F_LRO            32768   /* large receive offload */
> +#define NETIF_F_VLAN_TSO       65536   /* Supports TSO for VLANs */

This has the potential to get pretty messy.  How about making a
driver call back that the VLAN layer can invoke to get the supported
feature bits for the VLAN device?

You have to modify the driver to set the bit anyway so could as well
add a function instead of a bit.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

Herbert Xu wrote:
> Patrick McHardy <kaber@trash.net> wrote:
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index b11e6e1..3be4559 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -514,10 +514,11 @@ struct net_device
>> #define NETIF_F_NETNS_LOCAL    8192    /* Does not change network namespaces */
>> #define NETIF_F_MULTI_QUEUE    16384   /* Has multiple TX/RX queues */
>> #define NETIF_F_LRO            32768   /* large receive offload */
>> +#define NETIF_F_VLAN_TSO       65536   /* Supports TSO for VLANs */
> 
> This has the potential to get pretty messy.  How about making a
> driver call back that the VLAN layer can invoke to get the supported
> feature bits for the VLAN device?
> 
> You have to modify the driver to set the bit anyway so could as well
> add a function instead of a bit.


I thought about this myself, my second idea was to add a
mask for feature bits to be propagated. Unless there is a
need for the driver to determine them at runtime, thats
slightly simpler. The driver would do:

dev->vlan_features = NETIF_F_CSUM_ALL | ...

What do you think about this?

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Herbert Xu]

On Wed, May 21, 2008 at 06:28:26PM +0200, Patrick McHardy wrote:
>
> I thought about this myself, my second idea was to add a
> mask for feature bits to be propagated. Unless there is a
> need for the driver to determine them at runtime, thats
> slightly simpler. The driver would do:
> 
> dev->vlan_features = NETIF_F_CSUM_ALL | ...
> 
> What do you think about this?

Yes that sounds great!

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Waskiewicz Jr, Peter P]

> > I thought about this myself, my second idea was to add a mask for 
> > feature bits to be propagated. Unless there is a need for 
> the driver 
> > to determine them at runtime, thats slightly simpler. The 
> driver would 
> > do:
> > 
> > dev->vlan_features = NETIF_F_CSUM_ALL | ...
> > 
> > What do you think about this?
> 
> Yes that sounds great!

The issue is how does the driver know how to pull those flags off the
VLAN device when the parent has TSO or CSUM offload disabled?  The only
way I could come up with it was in my original patch in the drivers to
loop through the entire VLAN group array, and clear the flag on existing
devices.

Cheers,
-PJ Waskiewicz

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>>> I thought about this myself, my second idea was to add a mask for 
>>> feature bits to be propagated. Unless there is a need for 
>>>       
>> the driver 
>>     
>>> to determine them at runtime, thats slightly simpler. The 
>>>       
>> driver would 
>>     
>>> do:
>>>
>>> dev->vlan_features = NETIF_F_CSUM_ALL | ...
>>>
>>> What do you think about this?
>>>       
>> Yes that sounds great!
>>     
>
> The issue is how does the driver know how to pull those flags off the
> VLAN device when the parent has TSO or CSUM offload disabled?  The only
> way I could come up with it was in my original patch in the drivers to
> loop through the entire VLAN group array, and clear the flag on existing
> devices.
>   

Yes, thats also what my patch is doing. Not a big deal I guess,
we're doing that for all kinds of notifications already and
nobody ever complained.

RE: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Waskiewicz Jr, Peter P]

> Waskiewicz Jr, Peter P wrote:
> >>> I thought about this myself, my second idea was to add a mask for 
> >>> feature bits to be propagated. Unless there is a need for
> >>>       
> >> the driver
> >>     
> >>> to determine them at runtime, thats slightly simpler. The
> >>>       
> >> driver would
> >>     
> >>> do:
> >>>
> >>> dev->vlan_features = NETIF_F_CSUM_ALL | ...
> >>>
> >>> What do you think about this?
> >>>       
> >> Yes that sounds great!
> >>     
> >
> > The issue is how does the driver know how to pull those 
> flags off the 
> > VLAN device when the parent has TSO or CSUM offload disabled?  The 
> > only way I could come up with it was in my original patch in the 
> > drivers to loop through the entire VLAN group array, and clear the 
> > flag on existing devices.
> >   
> 
> Yes, thats also what my patch is doing. Not a big deal I 
> guess, we're doing that for all kinds of notifications 
> already and nobody ever complained.

I guess I misunderstood what you were suggesting to re-implement with
Herbert.  I like your current patch as-is, but if something different is
coming, I'll just sit tight for it.  I already spun the ixgbe, igb, and
e1000e patches to use the new interface, and they should be coming out
shortly.  But if we need to update them later, that's cool.

Cheers,
-PJ Waskiewicz

Re: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>> Waskiewicz Jr, Peter P wrote:
>>     
>>> The issue is how does the driver know how to pull those 
>>>       
>> flags off the 
>>     
>>> VLAN device when the parent has TSO or CSUM offload disabled?  The 
>>> only way I could come up with it was in my original patch in the 
>>> drivers to loop through the entire VLAN group array, and clear the 
>>> flag on existing devices.
>>>   
>>>       
>> Yes, thats also what my patch is doing. Not a big deal I 
>> guess, we're doing that for all kinds of notifications 
>> already and nobody ever complained.
>>     
>
> I guess I misunderstood what you were suggesting to re-implement with
> Herbert.  I like your current patch as-is, but if something different is
> coming, I'll just sit tight for it.  I already spun the ixgbe, igb, and
> e1000e patches to use the new interface, and they should be coming out
> shortly.  But if we need to update them later, that's cool.

The idea is simply to replace the NETIF_F_VLAN_TSO and checksumming
flag by a flag mask that specifies the features that may be propagated
to the VLAN devices.

The change to your driver will most likely be something like:

- dev->features |= NETIF_F_VLAN_TSO | ...
+ dev->vlan_features |= NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_CSUM_ALL | ...

I'll send the patch tommorrow, I'm currently busy with other things.

RE: [RFC, VLAN]: Propagate selected feature bits to VLAN devices

[Waskiewicz Jr, Peter P]

> The idea is simply to replace the NETIF_F_VLAN_TSO and 
> checksumming flag by a flag mask that specifies the features 
> that may be propagated to the VLAN devices.
> 
> The change to your driver will most likely be something like:
> 
> - dev->features |= NETIF_F_VLAN_TSO | ...
> + dev->vlan_features |= NETIF_F_TSO | NETIF_F_TSO6 | 
> NETIF_F_CSUM_ALL | ...
> 
> I'll send the patch tommorrow, I'm currently busy with other things.

Ok.  That sounds great.  Thanks Patrick.

-PJ Waskiewicz