On Wed, Apr 26, 2017 at 4:52 AM, Simon Horman <[email protected]> wrote: > On Tue, Apr 25, 2017 at 05:45:34PM -0700, Yi-Hung Wei wrote: >> 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. > > Thanks. > >> >> 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(). > > Understood, thanks for the detailed explanation. Perhaps a(nother) more > intuitive name could be used for HAVE_MPLS_HDR, f.e. > MPLS_NETWORK_HEADER_IS_L3? And/or a comment somewhere? > > ...
Sure, that make sense. I change HAVE_MPLS_HDR to MPLS_HEADER_IS_L3, and add comments at the mpls_hdr() backport. I will send out the v3 of patch 1. Thanks, -Yi-Hung _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
