Re: problem with MPLS and TSO/GSO

[Simon Horman <simon.horman@netronome.com>]

On Sun, Jul 31, 2016 at 12:07:10AM -0700, Roopa Prabhu wrote:
> On 7/27/16, 12:02 AM, zhuyj wrote:
> > On ubuntu16.04 server 64 bit
> > The attached script is run, the following will appear.
> >
> > Error: either "to" is duplicate, or "encap" is a garbage.
> 
> This maybe just because the iproute2 version on ubuntu does not
> support the route encap attributes yet.
> 
> [snip]
> 
> >
> > On Tue, Jul 26, 2016 at 12:39 AM, Lennert Buytenhek <buytenh@wantstofly.org>
> > wrote:
> >
> >> Hi!
> >>
> >> I am seeing pretty horrible TCP transmit performance (anywhere between
> >> 1 and 10 Mb/s, on a 10 Gb/s interface) when traffic is sent out over a
> >> route that involves MPLS labeling, and this seems to be due to an
> >> interaction between MPLS and TSO/GSO that causes all segmentable TCP
> >> frames that are MPLS-labeled to be dropped on egress.
> >>
> >> I initially ran into this issue with the ixgbe driver, but it is easily
> >> reproduced with veth interfaces, and the script attached below this
> >> email reproduces the issue.  The script configures three network
> >> namespaces: one that transmits TCP data (netperf) with MPLS labels,
> >> one that takes the MPLS traffic and pops the labels and forwards the
> >> traffic on, and one that receives the traffic (netserver).  When not
> >> using MPLS labeling, I get ~30000 Mb/s single-stream TCP performance
> >> in this setup on my test box, and with MPLS labeling, I get ~2 Mb/s.
> >>
> >> Some investigating shows that egress TCP frames that need to be
> >> segmented are being dropped in validate_xmit_skb(), which calls
> >> skb_gso_segment() which calls skb_mac_gso_segment() which returns
> >> -EPROTONOSUPPORT because we apparently didn't have the right kernel
> >> module (mpls_gso) loaded.
> >>
> >> (It's somewhat poor design, IMHO, to degrade network performance by
> >> 15000x if someone didn't load a kernel module they didn't know they
> >> should have loaded, and in a way that doesn't log any warnings or
> >> errors and can only be diagnosed by adding printk calls to net/core/
> >> and recompiling your kernel.)
> 
> Its possible that the right way to do this is to always auto select MPLS_GSO
> if MPLS_IPTUNNEL is selected. I am guessing this by looking at the
> openvswitch mpls Kconfig entries and comparing with MPLS_IPTUNNEL.
> will look some more.
> 
> >>
> >> (Also, I'm not sure why mpls_gso is needed when ixgbe seems to be
> >> able to natively do TSO on MPLS-labeled traffic, maybe because ixgbe
> >> doesn't advertise the necessary features in ->mpls_features?  But
> >> adding those bits doesn't seem to change much.)
> >>
> >> But, loading mpls_gso doesn't change much -- skb_gso_segment() then
> >> starts return -EINVAL instead, which is due to the
> >> skb_network_protocol() call in skb_mac_gso_segment() returning zero.
> >> And looking at skb_network_protocol(), I don't see how this is
> >> supposed to work -- skb->protocol is 0 at this point, and there is no
> >> way to figure out that what we are encapsulating is IP traffic, because
> >> unlike what is the case with VLAN tags, MPLS labels aren't followed by
> >> an inner ethertype that says what kind of traffic is in here, you have
> >> to have explicit knowledge of the payload type for MPLS.
> >>
> >> Any ideas?
> I was looking at the history of net/mpls/mpls_gso.c and the initial git log comment
> says that the driver expects the mpls tunnel driver to do a few things which I think
> might be the problem. I do see mpls_iptunnel.c setting the skb->protocol but not the
> skb->inner_protocol. wonder if fixing anything there will help ?.

If the inner protocol is not set then I don't think that segmentation can
function as there is (or at least was for the use case the code was added)
no way for the stack to know the protocol of the inner packet otherwise.

On another note I was recently poking around the code and I wonder if the
following may be needed (this was in the context of my under-construction
l3 tunnel work for OvS and it may only be needed in that context):

diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
index 2055e57ed1c3..113cba89653d 100644
--- a/net/mpls/mpls_gso.c
+++ b/net/mpls/mpls_gso.c
@@ -39,16 +39,18 @@ static struct sk_buff *mpls_gso_segment(struct sk_buff *skb,
 	mpls_features = skb->dev->mpls_features & features;
 	segs = skb_mac_gso_segment(skb, mpls_features);
 
-
-	/* Restore outer protocol. */
-	skb->protocol = mpls_protocol;
-
 	/* Re-pull the mac header that the call to skb_mac_gso_segment()
 	 * above pulled.  It will be re-pushed after returning
 	 * skb_mac_gso_segment(), an indirect caller of this function.
 	 */
 	__skb_pull(skb, skb->data - skb_mac_header(skb));
 
+	/* Restore outer protocol. */
+	skb->protocol = mpls_protocol;
+	if (!IS_ERR(segs))
+		for (skb = segs; skb; skb = skb->next)
+			skb->protocol = mpls_protocol;
+
 	return segs;
 }