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.
