Hi Simon, Thanks for your review. Please find my relies below. I will sent out v2 based on your review.
On Tue, Apr 25, 2017 at 12:43 AM, Simon Horman <[email protected]> wrote: > 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. Thanks for your suggestion. I pull two independent patches out of this patch in v2. Commit 48d2ab609b6b("net: mpls: Fixups for GSO") and 85de4a2101ac ("openvswitch: use mpls_hdr") are backported together since I am using the mpls_hdr() in the later commit to backport some logic in the first commit. > >> 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") Yes, you're right. Thanks for finding this bug. > >> 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. This patch uses HAVE_MPLS_HDR to determine if the kernel that ovs datapath is compiled with has the new or the old mpls_gso kernel module. Because starting from commit 48d2ab609b6b ("net: mpls: Fixups for GSO"), the mpls_gso kernel module relies on the fact that skb_network_header() points to the mpls header and skb_inner_network_header() points to the L3 header so that it can derive the length of mpls header correctly. However, the old mpls_gso kernel module assumes that the skb_network_header() points to the L3 header, and the old mpls_gso kernel module will misbehave if the ovs datapath marks the skb_network_header() in the new way since it will treat mpls header as the L3 header. Therefore, we only need to set the network_header in newer kernel. I use HAVE_MPLS_HDR to determine whether we have new or old mpls_gso module, cause I want to avoid using kernel version number, and since these two patches are related, and they are both in 4.9 kernel. The other part of the code does not need to be guarded by HAVE_MPLS_HDR mainly because of the following two reason. 1) It does no harm to the code outside of ovs datapath. For example, for older mpls_gso kernel module, it does not matter whether we set the inner network header or not. To make the datapath code more clean, I do not guard that type of code with HAVE_MPLS_HDR. There may be some cases I did not consider, but at least it work for the tests I perform. 2) The behavior is consistent within the ovs datapath. As long as the way we access the mpls header is consistent within ovs datapath. It should be fine whether we use skb_set_network_header() or skb_set_inner_network_header(). > > ... > >> @@ -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
