[ovs-dev] [PATCH net-next v11 5/6] openvswitch: add layer 3 flow/port support

[Simon Horman]

On Tue, Aug 09, 2016 at 08:47:40AM -0700, pravin shelar wrote:
> On Mon, Aug 8, 2016 at 8:17 AM, Simon Horman <simon.horman at netronome.com> wrote:

...

> > Hi Pravin,
> >
> > I have made an attempt to implement your suggestion to the extent that
> > I understand it. The following is an incremental change on top
> > of this patch-set. Does it move things closer to what you have in mind?
> >
> Following approach looks good to me. I have posted couple of comments.

Thanks, I am rather glad to hear that.

> > Light testing seems to indicate that it works for GSO skbs
> > received over both L3 and L2 GRE tunnels by OvS with both
> > IP-in-MPLS and IP (without MPLS) payloads.
> >
> 
> Thanks for testing it. Can you also add those tests to OVS kmod test suite?
> ..

Sure, I will look into doing that.
Am I correct in thinking Joe Stringer is the best person to contact if
I run into trouble there?

> > 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;
> > +
> 
> skb_mac_gso_segment() can also return NULL. Therefore segs should be
> checked for NULL case.

Sure, I will fix that.
I think that can be trivially resolved using IS_ERR_OR_NULL()

> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index 0001f651c934..424164222f1e 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> 
> ...
> > @@ -308,8 +319,8 @@ static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key)
> >  {
> >         skb_pull_rcsum(skb, ETH_HLEN);
> >         skb_reset_mac_header(skb);
> > -       skb->mac_len -= ETH_HLEN;
> >
> 
> I am not sure why this line is removed.

I will restore it.