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

[pravin shelar]

On Tue, Jul 19, 2016 at 5:02 PM, Simon Horman
<simon.horman at 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.horman at 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.horman at 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.horman at 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.