On Mon, Aug 8, 2016 at 8:17 AM, Simon Horman <simon.hor...@netronome.com> wrote:
> On Wed, Jul 20, 2016 at 11:06:37AM -0700, pravin shelar wrote:
>> On Tue, Jul 19, 2016 at 5:02 PM, Simon Horman
>> <simon.hor...@netronome.com> wrote:
>> > On Mon, Jul 18, 2016 at 03:34:52PM -0700, pravin shelar wrote:
>> >> On Sun, Jul 17, 2016 at 9:50 PM, Simon Horman
>> >> <simon.hor...@netronome.com> wrote:
>> >> > [CC Jiri Benc for portion regarding GRE]
>> >> >
>> >> > Hi Pravin,
>> >> >
>> >> > On Fri, Jul 15, 2016 at 02:07:37PM -0700, pravin shelar wrote:
>> >> >> On Wed, Jul 13, 2016 at 12:31 AM, Simon Horman
>> >> >> <simon.hor...@netronome.com> wrote:
>> >> >> > Hi Pravin,
>> >> >> >
>> >> >> > On Thu, Jul 07, 2016 at 01:54:15PM -0700, pravin shelar wrote:
>> >> >> >> On Wed, Jul 6, 2016 at 10:59 AM, Simon Horman
>> >> >> >> <simon.hor...@netronome.com> wrote:
>> >> >> >
>> >> >> > ...
>> >> >>
>> >> >> >
>> >> >> >> > diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> >> >> >> > index 0ea128eeeab2..86f2cfb19de3 100644
>> >> >> >> > --- a/net/openvswitch/flow.c
>> >> >> >> > +++ b/net/openvswitch/flow.c
>> >> >> >> ...
>> >> >> >>
>> >> >> >> > @@ -723,9 +729,17 @@ int ovs_flow_key_extract(const struct
>> >> >> >> > ip_tunnel_info *tun_info,
>> >> >> >> > key->phy.skb_mark = skb->mark;
>> >> >> >> > ovs_ct_fill_key(skb, key);
>> >> >> >> > key->ovs_flow_hash = 0;
>> >> >> >> > + key->phy.is_layer3 = skb->mac_len == 0;
>> >> >> >>
>> >> >> >> I do not think mac_len can be used. mac_header needs to be checked.
>> >> >> >> ...
>> >> >> >
>> >> >> > Yes, indeed. The update to use skb_mac_header_was_set() here
>> >> >> > accidently
>> >> >> > slipped into the following patch, sorry about that.
>> >> >> >
>> >> >> > With that change in place I believe that this patch is internally
>> >> >> > consistent because mac_header and mac_len are set correctly by the
>> >> >> > call to key_extract() which is called by ovs_flow_key_extract() just
>> >> >> > after where the excerpt above ends.
>> >> >> >
>> >> >> > That said, I do think that it is possible to rely on
>> >> >> > skb_mac_header_was_set
>> >> >> > throughout the datapath, including action processing etc... I have
>> >> >> > provided
>> >> >> > an incremental patch - which I created on top of this entire series
>> >> >> > - at
>> >> >> > the end of this email. If you prefer that approach I am happy to
>> >> >> > take it,
>> >> >> > though I do feel that using mac_len leads to slightly cleaner code.
>> >> >> > Let me
>> >> >> > know what you think.
>> >> >> >
>> >> >>
>> >> >>
>> >> >> I am not sure if you can use only mac_len to detect L3 packet. This
>> >> >> does not work with MPLS packets, mac_len is used to account MPLS
>> >> >> headers pushed on skb. Therefore in case of a MPLS header on L3
>> >> >> packet, mac_len would be non zero and we have to look at either
>> >> >> mac_header or some other metadata like is_layer3 flag from key to
>> >> >> check for L3 packet.
>> >> >
>> >> > At least within OvS mac_len does not include the length of the MPLS
>> >> > label
>> >> > stack. Rather, the MPLS label stack length is the difference between the
>> >> > end of (mac_header + mac_len) and network_header.
>> >> >
>> >> > So I think that the scheme does work as mac_len is 0 if there is no L2
>> >> > header regardless of if an MPLS label stack is present or not.
>> >> >
>> >>
>> >> I was thinking in overall networking stack rather than just ovs
>> >> datapath. I think we should have consistent method of detecting L3
>> >> packet. As commented in previous mail it could be achieved using
>> >> skb-protocol and device type.
>> >
>> > This is somewhat of a surprise to me. As far as I recall when MPLS support
>> > was added to OvS it and the accompanying support for MPLS GSO was the only
>> > MPLS support present in the kernel. And at the time the scheme developed by
>> > Jesse Gross, myself and others was as I describe above.
>> >
>> > Internally OvS relies on this scheme and in particular it is used
>> > by skb_mpls_header() to calculate the beginning of the MPLS label stack
>> > accurately in the presence of VLAN tags.
>> >
>> > Is it mpls_gso_segment() that you are concerned about?
>> > If so, perhaps the problem could be addressed there.
>>
>> Yes.
>> Can you read the comment I made in previous main in context of
>> function skb_mpls_header(). I have given rational for requested
>> change.
>
> 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.
> 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?
..
> 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.
> 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.
