Hi Yi-Hung, thanks for taking on this difficult piece of work and apologies for the delay in responding.
On Mon, Apr 03, 2017 at 04:24:36PM -0700, Yi-Hung Wei wrote: > This commit backports the following upstream commits to fix MPLS GSO in ovs > datapath. It has been tested on kernel 4.4 and 4.9. Thanks also for noting the versions this has been tested against. I expect there testing against other versions will show some residual problems but I think that fixing 4.4 and 4.9 is a good step forwards. I see that this patch backports 4 upstream patches. I am curious to know if you considered backporting them individually. That would have made reviewing a little easier for me. > Upstream commit: > commit 48d2ab609b6bbecb7698487c8579bc40de9d6dfa > Author: David Ahern <[email protected]> > Date: Wed Aug 24 20:10:44 2016 -0700 ... > Upstream commit: > commit f7d49bce8e741e1e6aa14ce4db1b6cea7e4be4e8 > Author: Jiri Benc <[email protected]> > Date: Fri Sep 30 19:08:05 2016 +0200 ... > Upstream commit: > commit 85de4a2101acb85c3b1dde465e84596ccca99f2c > Author: Jiri Benc <[email protected]> > Date: Fri Sep 30 19:08:07 2016 +0200 > > openvswitch: use mpls_hdr > > skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper > instead. > > Signed-off-by: Jiri Benc <[email protected]> > Acked-by: Pravin B Shelar <[email protected]> > Signed-off-by: David S. Miller <[email protected]> ... > Upstream commit: > commit c66549ffd666605831abf6cf19ce0571ad868e39 > Author: Jiri Benc <[email protected]> > Date: Wed Oct 5 15:01:57 2016 +0200 ... > diff --git a/acinclude.m4 b/acinclude.m4 > index 744d8f89525c..82ca4a28015c 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -479,6 +479,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ > [OVS_GREP_IFELSE([$KSRC/include/net/dst_cache.h], > [dst_cache], > > [OVS_DEFINE([USE_UPSTREAM_TUNNEL])])])]) > > + OVS_GREP_IFELSE([$KSRC/include/linux/mpls.h], [mpls_hdr]) Should the path be $KSRC/include/net/mpls.h? I am looking at 9095e10edd28 ("mpls: move mpls_hdr to a common location") > OVS_GREP_IFELSE([$KSRC/include/linux/net.h], [sock_create_kern.*net], > [OVS_DEFINE([HAVE_SOCK_CREATE_KERN_NET])]) > OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [ndo_fill_metadata_dst]) > diff --git a/datapath/actions.c b/datapath/actions.c > index 06080b24ea5a..ecc5136a3529 100644 > --- a/datapath/actions.c > +++ b/datapath/actions.c ... > @@ -169,20 +170,26 @@ static int push_mpls(struct sk_buff *skb, struct > sw_flow_key *key, > if (skb_cow_head(skb, MPLS_HLEN) < 0) > return -ENOMEM; > > + if (!ovs_skb_get_inner_protocol(skb)) { > + skb_set_inner_network_header(skb, skb->mac_len); > + ovs_skb_set_inner_protocol(skb, skb->protocol); > + } > + > skb_push(skb, MPLS_HLEN); > memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb), > skb->mac_len); > skb_reset_mac_header(skb); > +#ifdef HAVE_MPLS_HDR > + skb_set_network_header(skb, skb->mac_len); > +#endif It is not clear to me why this call to skb_set_network_header() is guarded by HAVE_MPLS_HDR here and moreover not elsewhere in this patch. ... > @@ -198,21 +205,22 @@ static int pop_mpls(struct sk_buff *skb, struct > sw_flow_key *key, > if (unlikely(err)) > return err; > > - skb_postpull_rcsum(skb, skb_mpls_header(skb), MPLS_HLEN); > + skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN); > > memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb), > skb->mac_len); > > __skb_pull(skb, MPLS_HLEN); > skb_reset_mac_header(skb); > + skb_set_network_header(skb, skb->mac_len); ... > @@ -707,6 +715,12 @@ static int ovs_vport_output(OVS_VPORT_OUTPUT_PARAMS) > skb_postpush_rcsum(skb, skb->data, data->l2_len); > skb_reset_mac_header(skb); > > + if (eth_p_mpls(skb->protocol)) { > + skb->inner_network_header = skb->network_header; > + skb_set_network_header(skb, data->network_offset); > + skb_reset_mac_len(skb); > + } > + ... > diff --git a/datapath/flow.c b/datapath/flow.c > index 2bc1ad0d5304..1d40b2400406 100644 > --- a/datapath/flow.c > +++ b/datapath/flow.c ... > @@ -679,12 +674,12 @@ static int key_extract(struct sk_buff *skb, struct > sw_flow_key *key) > if (unlikely(error)) > return 0; > > - memcpy(&lse, skb_network_header(skb), MPLS_HLEN); > + memcpy(&lse, skb_inner_network_header(skb), MPLS_HLEN); > > if (stack_len == MPLS_HLEN) > memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN); > > - skb_set_network_header(skb, skb->mac_len + stack_len); > + skb_set_inner_network_header(skb, skb->mac_len + > stack_len); > if (lse & htonl(MPLS_LS_S_MASK)) > break; > ... _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
