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.hor...@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.
