Re: [PATCH net-next v11 5/6] openvswitch: add layer 3 flow/port support
From: pravin shelar <hidden>
Date: 2016-08-09 15:47:40
On Mon, Aug 8, 2016 at 8:17 AM, Simon Horman [off-list ref] wrote:
On Wed, Jul 20, 2016 at 11:06:37AM -0700, pravin shelar wrote:quoted
On Tue, Jul 19, 2016 at 5:02 PM, Simon Horman [off-list ref] wrote:quoted
On Mon, Jul 18, 2016 at 03:34:52PM -0700, pravin shelar wrote:quoted
On Sun, Jul 17, 2016 at 9:50 PM, Simon Horman [off-list ref] wrote:quoted
[CC Jiri Benc for portion regarding GRE] Hi Pravin, On Fri, Jul 15, 2016 at 02:07:37PM -0700, pravin shelar wrote:quoted
On Wed, Jul 13, 2016 at 12:31 AM, Simon Horman [off-list ref] wrote:quoted
Hi Pravin, On Thu, Jul 07, 2016 at 01:54:15PM -0700, pravin shelar wrote:quoted
On Wed, Jul 6, 2016 at 10:59 AM, Simon Horman [off-list ref] wrote:...quoted
quoted
quoted
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...quoted
@@ -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? ..
quoted hunk
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.
quoted hunk
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
...
quoted hunk
@@ -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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev