Skip to contentSkip to conversation

: Propagate selected feature bits to VLAN devices

RFC Applied Patrick McHardy 6 participants 24 messages May 20, 2008 – May 22, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Patrick McHardy

author
#0 lore raw

RFC 1/1 : Propagate selected feature bits to VLAN devices

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

commit b01a0b6b4a0463fe896e166d0782f0daca8c69ff

Author: Patrick McHardy <[email protected]>
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 <[email protected]>
include/linux/netdevice.h +3 -2
index b11e6e1..3be4559 100644
@@ -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)
net/8021q/vlan.c +25 -0
index 2a739ad..1f172f7 100644
@@ -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++) {
net/8021q/vlan_dev.c +1 -0
index c961f08..3705d5a 100644
@@ -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;
author
#1 lore raw
Quoted 2 lines · Patrick McHardy

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

Oops, wrong Peter, please ignore :)

#2 lore raw
In reply to Patrick McHardy
Quoted 5 lines · Patrick McHardy
include/linux/netdevice.h +0 -0
index b11e6e1..3be4559 100644

include/linux/netdevice.h struct net_device · line 521

Quoted 10 lines
include/linux/netdevice.h +3 -2
@@ -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.

Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
author
#3 lore raw
Quoted 11 lines · Ben Hutchings
Quoted 10 lines · Patrick McHardy
include/linux/netdevice.h +1 -0
index b11e6e1..3be4559 100644
@@ -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 */
Quoted 9 lines

include/linux/netdevice.h struct net_device · line 521

Quoted 5 lines
include/linux/netdevice.h +2 -2
 	/* 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).

#4 lore raw
Quoted 25 lines · Patrick McHardy
Quoted 13 lines · Ben Hutchings
Quoted 12 lines · Patrick McHardy
include/linux/netdevice.h +0 -0
index b11e6e1..3be4559 100644
@@ -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 */

Quoted 9 lines

include/linux/netdevice.h struct net_device · line 521

Quoted 5 lines
     /* Segmentation offload features */
include/linux/netdevice.h +2 -2
-#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?

Quoted 3 lines

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

Quoted 5 lines
Signature · 4 lines
-- 
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Ben Greear <[email protected]> Candela Technologies Inc  http://www.candelatech.com
#5 lore raw
Quoted 27 lines · Ben Greear
Quoted 26 lines · Patrick McHardy
Quoted 23 lines · Ben Hutchings
Quoted 10 lines · Patrick McHardy
include/linux/netdevice.h +0 -0
index b11e6e1..3be4559 100644
@@ -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 */

include/linux/netdevice.h struct net_device · line 521

Quoted 7 lines
include/linux/netdevice.h +3 -2
+#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?

Quoted 7 lines
Quoted 3 lines

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.

Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
author
#6 lore raw
Quoted 10 lines · Ben Hutchings
Quoted 9 lines · Ben Greear
Quoted 8 lines · Patrick McHardy
Quoted 3 lines · Ben Hutchings

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

Quoted 8 lines
Quoted 2 lines

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.

#7 lore raw
In reply to Ben Hutchings
Quoted 8 lines · Ben Hutchings

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

Signature · 1 line
Ben Greear <[email protected]> Candela Technologies Inc  http://www.candelatech.com
#8 lore raw
Quoted 3 lines

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

author
#9 lore raw
Quoted 5 lines · Peter P
Quoted 1 line

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

Quoted 3 lines

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

Thanks.

#10 lore raw
In reply to Patrick McHardy

Patrick McHardy wrote:
[...]

Quoted 4 lines
net/8021q/vlan.c +0 -0
@@ -382,6 +382,19 @@ static void vlan_sync_address(struct net_device *dev,
 	memcpy(vlan->real_dev_addr, dev->dev_addr, ETH_ALEN);

net/8021q/vlan.c static void vlan_sync_address(struct net_device *dev, · line 392

Quoted 10 lines
net/8021q/vlan.c +8 -0
 }
 
+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?

Quoted 7 lines
net/8021q/vlan.c +4 -0
+	if (old_features != vlandev->features)
+		netdev_features_change(vlandev);
+}
+
 static void __vlan_device_event(struct net_device *dev, unsigned long event)
 {
 	switch (event) {

[...]

net/8021q/vlan_dev.c static int vlan_dev_init(struct net_device *dev) · line 668

Quoted 12 lines
net/8021q/vlan_dev.c +1 -0
index c961f08..3705d5a 100644
@@ -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.

Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
author
#11 lore raw
Quoted 8 lines · Ben Hutchings

Review

Quoted 4 lines · Patrick McHardy
(quoted diff) +3 -0
+	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.

Quoted 8 lines

Review

Quoted 2 lines
(quoted diff) +1 -0
+	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.

commit fd467a0422c9c8ddd05c53c4f91d4af71c8fb9ff

Author: Patrick McHardy <[email protected]>
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 <[email protected]>
include/linux/netdevice.h +4 -2
index b11e6e1..2b02664 100644
@@ -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)
net/8021q/vlan.c +30 -0
index 2a739ad..20fb6fd 100644
@@ -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++) {
net/8021q/vlan.h +2 -0
index 5229a72..7962569 100644
@@ -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
net/8021q/vlan_dev.c +5 -0
index c961f08..b1cfbaa 100644
@@ -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;
#12 lore raw
Quoted 11 lines
Quoted 2 lines

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

Yes, thanks.

Quoted 1 line

Review

Quoted 1 line
(quoted diff) +1 -0
+	dev->features = real_dev->features & (NETIF_F_TSO |

NETIF_F_TSO6);

Quoted 6 lines
Quoted 1 line

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

Quoted 6 lines

worth adding a flag for that.

Quoted 1 line

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
<[email protected]>

maintainer Applied
#14 lore raw
In reply to Patrick McHardy

From: Patrick McHardy <[email protected]>
Date: Tue, 20 May 2008 20:31:48 +0200

Quoted 4 lines

commit fd467a0422c9c8ddd05c53c4f91d4af71c8fb9ff

Author: Patrick McHardy <[email protected]>
Date:   Tue May 20 20:31:40 2008 +0200
Quoted 8 lines

[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 <[email protected]>

Applied, thanks Patrick!

author
#15 lore raw
Quoted 9 lines · David Miller

From: Patrick McHardy <[email protected]>
Date: Tue, 20 May 2008 20:31:48 +0200

Quoted 6 lines

commit fd467a0422c9c8ddd05c53c4f91d4af71c8fb9ff

Author: Patrick McHardy <[email protected]>
Date:   Tue May 20 20:31:40 2008 +0200

[VLAN]: Propagate selected feature bits to VLAN devices

Quoted 8 lines
Quoted 6 lines

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 <[email protected]>

Applied, thanks Patrick!

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

author
#13 lore raw
In reply to Waskiewicz Jr, Peter P
Quoted 10 lines · Peter P
Quoted 10 lines
Quoted 2 lines

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

Yes, thanks.

Quoted 1 line

Review

Quoted 1 line
(quoted diff) +1 -0
+	dev->features = real_dev->features & (NETIF_F_TSO |

NETIF_F_TSO6);

Quoted 3 lines
Quoted 1 line

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.

Quoted 1 line
Quoted 8 lines
Quoted 1 line

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
<[email protected]>

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

#16 lore raw
In reply to Patrick McHardy

include/linux/netdevice.h struct net_device · line 517

Quoted 10 lines · Patrick McHardy
include/linux/netdevice.h +1 -0
index b11e6e1..3be4559 100644
@@ -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,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#17 lore raw
Quoted 15 lines · Herbert Xu

include/linux/netdevice.h struct net_device · line 517

Quoted 9 lines · Patrick McHardy
include/linux/netdevice.h +1 -0
index b11e6e1..3be4559 100644
@@ -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?

Quoted 2 lines

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?

#18 lore raw
Quoted 9 lines · Patrick McHardy

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,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#19 lore raw
Quoted 4 lines
Quoted 2 lines

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

Quoted 1 line

to determine them at runtime, thats slightly simpler. The

Quoted 8 lines

driver would

Quoted 5 lines

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

author
#20 lore raw
Quoted 14 lines · Peter P
Quoted 14 lines
Quoted 2 lines

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

Quoted 2 lines

to determine them at runtime, thats slightly simpler. The

driver would

Quoted 6 lines

do:

dev->vlan_features = NETIF_F_CSUM_ALL | ...

What do you think about this?

Quoted 9 lines
Quoted 2 lines

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.

#21 lore raw
Quoted 27 lines
Quoted 11 lines · Peter P
Quoted 11 lines
Quoted 3 lines

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

Quoted 2 lines

to determine them at runtime, thats slightly simpler. The

driver would

Quoted 2 lines

do:

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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

Quoted 5 lines

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.

Quoted 3 lines

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

author
#22 lore raw
Quoted 9 lines · Peter P
Quoted 9 lines
Quoted 2 lines · Peter P

The issue is how does the driver know how to pull those

flags off the

Quoted 3 lines

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.

Quoted 8 lines
Quoted 2 lines

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.

#23 lore raw
Quoted 11 lines

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