[PATCH net-next 0/1] fix vlan transmit performance

Hi,

When doing some 10GbE perf measurments on very old athlon64
machines with myri10ge, I noticed that I was seeing CPU saturation
when transmitting vlan tagged traffic.  I think I traced the
problem to this line in netif_skb_features():

	features &= (skb->dev->vlan_features | NETIF_F_HW_VLAN_TX);

The problem seems to be that packets travel through this function
twice, first on their way to the vlan xmit handler, and then on
their way to the backing device's xmit handler.  On the first
pass, "skb->dev" is the vlan device, and skb->dev->vlan_features
is blank.  This causes netif_skb_features() to strip the offloads
away.

The following patch (just copy dev->features to dev->vlan_features in
vlan_dev_init()) seems to be the simplest way to fix it. Perhaps this
is wrong, and there is a better way?  Given that this has apparently
been broken for nearly 2 years (since f01a5236), I'm worried that
either I'm doing something wrong in myri10ge, or that I'm missing
something in general.

At any rate, performance jumps from 5.6Gb/s with one CPU entirely
saturated on the sender, to 9Gb/s with idle time:

Recv   Send    Send                          Utilization       Service Demand
Socket Socket  Message  Elapsed              Send     Recv     Send    Recv
Size   Size    Size     Time     Throughput  local    remote   local   remote
bytes  bytes   bytes    secs.    10^6bits/s  % S      % S      us/KB   us/KB

before:
 87380  65536  65536    10.00      5660.66   25.54    51.41    1.478   1.488  

after:
 87380  65536  65536    10.00      9081.39   15.66    76.42    0.565   1.379  



Andrew Gallatin (1):
  vlan: restore offload use on vlan transmit

 net/8021q/vlan_dev.c |    1 +
 1 file changed, 1 insertion(+)

-- 
1.7.9.5

This patch copies the vlan device's dev->features to its
dev->vlan_features, which allows packets to arrive at
vlan_dev_hard_start_xmit() with their offloads intact.

When a packet destined for a vlan interface is transmitted, it passes
through dev_hard_start_xmit() (and netif_skb_features()) twice.  First
on its way to vlan_dev_hard_start_xmit(), and then again on its way to
the backing device's xmit handler. If the vlan device does not setup
its dev->vlan_features, then netif_skb_features() will strip the
features on the first trip through it (on the way to
vlan_dev_hard_start_xmit()).

Signed-off-by: Andrew Gallatin <redacted>
---
 net/8021q/vlan_dev.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 4a6d31a..9d4b3c9 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c

@@ -559,6 +559,7 @@ static int vlan_dev_init(struct net_device *dev)
 
 	dev->features |= real_dev->vlan_features | NETIF_F_LLTX;
 	dev->gso_max_size = real_dev->gso_max_size;
+	dev->vlan_features = dev->features;
 
 	/* ipv6 shared card related stuff */
 	dev->dev_id = real_dev->dev_id;

-- 
1.7.9.5

On Thu, 2012-12-06 at 15:54 -0500, Andrew Gallatin wrote:

Hi,

When doing some 10GbE perf measurments on very old athlon64
machines with myri10ge, I noticed that I was seeing CPU saturation
when transmitting vlan tagged traffic.  I think I traced the
problem to this line in netif_skb_features():

	features &= (skb->dev->vlan_features | NETIF_F_HW_VLAN_TX);

The problem seems to be that packets travel through this function
twice, first on their way to the vlan xmit handler, and then on
their way to the backing device's xmit handler.  On the first
pass, "skb->dev" is the vlan device, and skb->dev->vlan_features
is blank.  This causes netif_skb_features() to strip the offloads
away.

On the first pass through dev_hard_start_xmit(), we should have:

	if (protocol == htons(ETH_P_8021Q)) {		// false
		...
	} else if (!vlan_tx_tag_present(skb)) {		// true
		return harmonize_features(skb, protocol, features);
	}
	// not executed
	features &= (skb->dev->vlan_features | NETIF_F_HW_VLAN_TX);
	...

Then the VLAN tag gets inserted in vlan_dev_hard_start_xmit(), and for
the second pass we used the vlan_features of the underlying device.

However, when reorder_hdr is off and the underlying device doesn't have
NETIF_F_VLAN_TX, the VLAN tag is inserted before the first pass, in
vlan_dev_hard_header().  Then, as you've seen, all TX offload features
get disabled.  (Prior to Linux 2.6.37 this configuration was
*completely* broken as the TX path didn't make consistent decisions
about whether offload features could be used.)

My answer would be: leave reorder_hdr enabled per default.  In fact,
perhaps the VLAN driver's TX path should ignore it if the underlying
device has non-zero vlan_features.  (It's already ignored if the
underlying device has NETIF_F_VLAN_TX.)

The following patch (just copy dev->features to dev->vlan_features in
vlan_dev_init()) seems to be the simplest way to fix it. Perhaps this
is wrong, and there is a better way?

[...]

It's wrong, because those features would then be recursively transferred
to further stacked VLAN devices.

Ben.

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

On Thu, 2012-12-06 at 15:54 -0500, Andrew Gallatin wrote:

To repeat myself: no, this will result in the features being transferred
to VLAN devices stacked on top of this one (to an arbitrary depth!).

Ben.

 	/* ipv6 shared card related stuff */
 	dev->dev_id = real_dev->dev_id;

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

On 12/06/12 18:42, Ben Hutchings wrote:

My answer would be: leave reorder_hdr enabled per default.  In fact,
perhaps the VLAN driver's TX path should ignore it if the underlying
device has non-zero vlan_features.  (It's already ignored if the
underlying device has NETIF_F_VLAN_TX.)

Thanks for the feedback.  I knew I was missing something!
I will try your approach, unless you beat me to it.
I assume sfc is also impacted by this..?

Drew

On 12/06/12 18:42, Ben Hutchings wrote:

On Thu, 2012-12-06 at 15:54 -0500, Andrew Gallatin wrote:

<..>

quoted

The following patch (just copy dev->features to dev->vlan_features in
vlan_dev_init()) seems to be the simplest way to fix it. Perhaps this
is wrong, and there is a better way?

[...]

It's wrong, because those features would then be recursively transferred
to further stacked VLAN devices.

The more I play with it & try various combinations, the more
I realize how tricky stacked vlans makes things,
so I think its best for me to just leave this alone..

Sorry for the noise.

Drew