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
> <simon.hor...@netronome.com> 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 <d...@cumulusnetworks.com>
> >> Date: Wed Aug 24 20:10:44 2016 -0700
> >
> > ...
> >
> >> Upstream commit:
> >> commit f7d49bce8e741e1e6aa14ce4db1b6cea7e4be4e8
> >> Author: Jiri Benc <jb...@redhat.com>
> >> Date: Fri Sep 30 19:08:05 2016 +0200
> >
> > ...
> >
> >> Upstream commit:
> >> commit 85de4a2101acb85c3b1dde465e84596ccca99f2c
> >> Author: Jiri Benc <jb...@redhat.com>
> >> 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 <jb...@redhat.com>
> >> Acked-by: Pravin B Shelar <pshe...@ovn.org>
> >> Signed-off-by: David S. Miller <da...@davemloft.net>
> >
> > ...
> >
> >> Upstream commit:
> >> commit c66549ffd666605831abf6cf19ce0571ad868e39
> >> Author: Jiri Benc <jb...@redhat.com>
> >> 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?
...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
