Re: [ovs-dev] [PATCH v1] flow.c: Refactor the key_extract function in parsing frame.

[Gao Zhenyu]

Hi Jarno,

Thanks for the suggestion,  I will try to get a performance number and
submit it into Linux first. :)

Thanks
Zhenyu Gao

2017-04-22 4:24 GMT+08:00 Jarno Rajahalme :

> As a policy, Linux kernel datapath changes other than backports need to go
> to upstream Linux first, new features to net-next tree, and bug fixes to
> net tree. See the documentation file ‘backporting-patches.rst’ in directory
> 'Documentation/internals/contributing/‘ of the OVS tree for more detailed
> description of the process.
>
> Also, the commit message should contain a clear motivation for the change.
> Changes that enhance readability may adversely affect datapath performance,
> so having a report on performance testing would be helpful in determining
> whether to apply the change.
>
> Regards,
>
>   Jarno
>
> > On Apr 21, 2017, at 12:21 AM, Zhenyu Gao 
> wrote:
> >
> > 1. Consume switch/case to judge type of frame instead of using if/else.
> > 2. Add parse_ipv4hdr for ipv4 frame header parsing.
> >
> > Signed-off-by: Zhenyu Gao 
> > ---
> > datapath/flow.c | 230 --
> --
> > 1 file changed, 117 insertions(+), 113 deletions(-)
> >
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index 2bc1ad0..0b35de6 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
> > sizeof(struct icmphdr));
> > }
> >
> > +/**
> > +  * Parse ipv4 header from an Ethernet frame.
> > +  * Return ipv4 header length if successful, otherwise a negative errno
> value.
> > +  */
> > +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> > +{
> > + int err;
> > + struct iphdr *nh;
> > + __be16 offset;
> > +
> > + err = check_iphdr(skb);
> > + if (unlikely(err))
> > + return err;
> > +
> > + nh = ip_hdr(skb);
> > + key->ipv4.addr.src = nh->saddr;
> > + key->ipv4.addr.dst = nh->daddr;
> > +
> > + key->ip.proto = nh->protocol;
> > + key->ip.tos = nh->tos;
> > + key->ip.ttl = nh->ttl;
> > +
> > + offset = nh->frag_off & htons(IP_OFFSET);
> > + if (offset) {
> > + key->ip.frag = OVS_FRAG_TYPE_LATER;
> > + } else {
> > + if (nh->frag_off & htons(IP_MF) ||
> > + skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> > + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> > + } else {
> > + key->ip.frag = OVS_FRAG_TYPE_NONE;
> > + }
> > + }
> > + return ip_hdrlen(skb);
> > +}
> > +
> > +/**
> > +  * Parse ipv6 header from an Ethernet frame.
> > +  * Return ipv6 header length if successful, otherwise a negative errno
> value.
> > +  */
> > static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
> > {
> >   unsigned int nh_ofs = skb_network_offset(skb);
> > @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb,
> struct sw_flow_key *key)
> >   else
> >   key->ip.frag = OVS_FRAG_TYPE_FIRST;
> >   } else {
> > - key->ip.frag = OVS_FRAG_TYPE_NONE;
> > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> > + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> > + else
> > + key->ip.frag = OVS_FRAG_TYPE_NONE;
> >   }
> >
> >   /* Delayed handling of error in ipv6_skip_exthdr() as it
> > @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> >   key->eth.type = skb->protocol;
> >
> >   /* Network layer. */
> > - if (key->eth.type == htons(ETH_P_IP)) {
> > - struct iphdr *nh;
> > - __be16 offset;
> > + switch(key->eth.type) {
> > + case htons(ETH_P_IP):
> > + case htons(ETH_P_IPV6): {
> > + int nh_len;
> > + if (key->eth.type == htons(ETH_P_IP)) {
> > + nh_len = parse_ipv4hdr(skb, key);
> > + } else {
> > + nh_len = parse_ipv6hdr(skb, key);
> > + }
> >
> > - error = check_iphdr(skb);
> > - if (unlikely(error)) {
> > - memset(>ip, 0, sizeof(key->ip));
> > - memset(>ipv4, 0, sizeof(key->ipv4));
> > - if (error == -EINVAL) {
> > + if (unlikely(nh_len < 0)) {
> > + switch (nh_len) {
> > + case -EINVAL:
> > + memset(>ip, 0, sizeof(key->ip));
> > + if (key->eth.type == htons(ETH_P_IP)) {
> > + memset(>ipv4.addr, 0,
> sizeof(key->ipv4.addr));
> > + } else {
> > + memset(>ipv6.addr, 0,
> sizeof(key->ipv6.addr));
> > + }
> > + 

Re: [ovs-dev] [PATCH v1] flow.c: Refactor the key_extract function in parsing frame.

[Jarno Rajahalme]

As a policy, Linux kernel datapath changes other than backports need to go to 
upstream Linux first, new features to net-next tree, and bug fixes to net tree. 
See the documentation file ‘backporting-patches.rst’ in directory 
'Documentation/internals/contributing/‘ of the OVS tree for more detailed 
description of the process.

Also, the commit message should contain a clear motivation for the change. 
Changes that enhance readability may adversely affect datapath performance, so 
having a report on performance testing would be helpful in determining whether 
to apply the change.

Regards,

  Jarno

> On Apr 21, 2017, at 12:21 AM, Zhenyu Gao  wrote:
> 
> 1. Consume switch/case to judge type of frame instead of using if/else.
> 2. Add parse_ipv4hdr for ipv4 frame header parsing.
> 
> Signed-off-by: Zhenyu Gao 
> ---
> datapath/flow.c | 230 
> 1 file changed, 117 insertions(+), 113 deletions(-)
> 
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2bc1ad0..0b35de6 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
> sizeof(struct icmphdr));
> }
> 
> +/**
> +  * Parse ipv4 header from an Ethernet frame.
> +  * Return ipv4 header length if successful, otherwise a negative errno 
> value.
> +  */
> +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> + int err;
> + struct iphdr *nh;
> + __be16 offset;
> +
> + err = check_iphdr(skb);
> + if (unlikely(err))
> + return err;
> +
> + nh = ip_hdr(skb);
> + key->ipv4.addr.src = nh->saddr;
> + key->ipv4.addr.dst = nh->daddr;
> +
> + key->ip.proto = nh->protocol;
> + key->ip.tos = nh->tos;
> + key->ip.ttl = nh->ttl;
> +
> + offset = nh->frag_off & htons(IP_OFFSET);
> + if (offset) {
> + key->ip.frag = OVS_FRAG_TYPE_LATER;
> + } else {
> + if (nh->frag_off & htons(IP_MF) ||
> + skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + } else {
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
> + }
> + }
> + return ip_hdrlen(skb);
> +}
> +
> +/**
> +  * Parse ipv6 header from an Ethernet frame.
> +  * Return ipv6 header length if successful, otherwise a negative errno 
> value.
> +  */
> static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
> {
>   unsigned int nh_ofs = skb_network_offset(skb);
> @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct 
> sw_flow_key *key)
>   else
>   key->ip.frag = OVS_FRAG_TYPE_FIRST;
>   } else {
> - key->ip.frag = OVS_FRAG_TYPE_NONE;
> + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + else
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
>   }
> 
>   /* Delayed handling of error in ipv6_skip_exthdr() as it
> @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct 
> sw_flow_key *key)
>   key->eth.type = skb->protocol;
> 
>   /* Network layer. */
> - if (key->eth.type == htons(ETH_P_IP)) {
> - struct iphdr *nh;
> - __be16 offset;
> + switch(key->eth.type) {
> + case htons(ETH_P_IP):
> + case htons(ETH_P_IPV6): {
> + int nh_len;
> + if (key->eth.type == htons(ETH_P_IP)) {
> + nh_len = parse_ipv4hdr(skb, key);
> + } else {
> + nh_len = parse_ipv6hdr(skb, key);
> + }
> 
> - error = check_iphdr(skb);
> - if (unlikely(error)) {
> - memset(>ip, 0, sizeof(key->ip));
> - memset(>ipv4, 0, sizeof(key->ipv4));
> - if (error == -EINVAL) {
> + if (unlikely(nh_len < 0)) {
> + switch (nh_len) {
> + case -EINVAL:
> + memset(>ip, 0, sizeof(key->ip));
> + if (key->eth.type == htons(ETH_P_IP)) {
> + memset(>ipv4.addr, 0, 
> sizeof(key->ipv4.addr));
> + } else {
> + memset(>ipv6.addr, 0, 
> sizeof(key->ipv6.addr));
> + }
> + /* fall-through */
> + case -EPROTO:
>   skb->transport_header = skb->network_header;
>   error = 0;
> + break;
> + default:
> + error = nh_len;
>   }
>   return error;
>   }
> 
> - nh = ip_hdr(skb);

Re: [ovs-dev] [PATCH v1] flow.c: Refactor the key_extract function in parsing frame.

[Greg Rose]

On Fri, 2017-04-21 at 00:21 -0700, Zhenyu Gao wrote:
> 1. Consume switch/case to judge type of frame instead of using if/else.
> 2. Add parse_ipv4hdr for ipv4 frame header parsing.
> 

I think this patch addresses too many issues and should be broken up
into at least two different patches with appropriate subjects and commit
comments for each.

Thanks,

- Greg

> Signed-off-by: Zhenyu Gao 
> ---
>  datapath/flow.c | 230 
> 
>  1 file changed, 117 insertions(+), 113 deletions(-)
> 
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2bc1ad0..0b35de6 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
> sizeof(struct icmphdr));
>  }
>  
> +/**
> +  * Parse ipv4 header from an Ethernet frame.
> +  * Return ipv4 header length if successful, otherwise a negative errno 
> value.
> +  */
> +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> + int err;
> + struct iphdr *nh;
> + __be16 offset;
> +
> + err = check_iphdr(skb);
> + if (unlikely(err))
> + return err;
> +
> + nh = ip_hdr(skb);
> + key->ipv4.addr.src = nh->saddr;
> + key->ipv4.addr.dst = nh->daddr;
> +
> + key->ip.proto = nh->protocol;
> + key->ip.tos = nh->tos;
> + key->ip.ttl = nh->ttl;
> +
> + offset = nh->frag_off & htons(IP_OFFSET);
> + if (offset) {
> + key->ip.frag = OVS_FRAG_TYPE_LATER;
> + } else {
> + if (nh->frag_off & htons(IP_MF) ||
> + skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + } else {
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
> + }
> + }
> + return ip_hdrlen(skb);
> +}
> +
> +/**
> +  * Parse ipv6 header from an Ethernet frame.
> +  * Return ipv6 header length if successful, otherwise a negative errno 
> value.
> +  */
>  static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
>  {
>   unsigned int nh_ofs = skb_network_offset(skb);
> @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct 
> sw_flow_key *key)
>   else
>   key->ip.frag = OVS_FRAG_TYPE_FIRST;
>   } else {
> - key->ip.frag = OVS_FRAG_TYPE_NONE;
> + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + else
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
>   }
>  
>   /* Delayed handling of error in ipv6_skip_exthdr() as it
> @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct 
> sw_flow_key *key)
>   key->eth.type = skb->protocol;
>  
>   /* Network layer. */
> - if (key->eth.type == htons(ETH_P_IP)) {
> - struct iphdr *nh;
> - __be16 offset;
> + switch(key->eth.type) {
> + case htons(ETH_P_IP):
> + case htons(ETH_P_IPV6): {
> + int nh_len;
> + if (key->eth.type == htons(ETH_P_IP)) {
> + nh_len = parse_ipv4hdr(skb, key);
> + } else {
> + nh_len = parse_ipv6hdr(skb, key);
> + }
>  
> - error = check_iphdr(skb);
> - if (unlikely(error)) {
> - memset(>ip, 0, sizeof(key->ip));
> - memset(>ipv4, 0, sizeof(key->ipv4));
> - if (error == -EINVAL) {
> + if (unlikely(nh_len < 0)) {
> + switch (nh_len) {
> + case -EINVAL:
> + memset(>ip, 0, sizeof(key->ip));
> + if (key->eth.type == htons(ETH_P_IP)) {
> + memset(>ipv4.addr, 0, 
> sizeof(key->ipv4.addr));
> + } else {
> + memset(>ipv6.addr, 0, 
> sizeof(key->ipv6.addr));
> + }
> + /* fall-through */
> + case -EPROTO:
>   skb->transport_header = skb->network_header;
>   error = 0;
> + break;
> + default:
> + error = nh_len;
>   }
>   return error;
>   }
>  
> - nh = ip_hdr(skb);
> - key->ipv4.addr.src = nh->saddr;
> - key->ipv4.addr.dst = nh->daddr;
> -
> - key->ip.proto = nh->protocol;
> - key->ip.tos = nh->tos;
> - key->ip.ttl = nh->ttl;
> -
> - offset = nh->frag_off & htons(IP_OFFSET);
> - if (offset) {
> - key->ip.frag = OVS_FRAG_TYPE_LATER;
> + if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
> 

[ovs-dev] [PATCH v1] flow.c: Refactor the key_extract function in parsing frame.

[Zhenyu Gao]

1. Consume switch/case to judge type of frame instead of using if/else.
2. Add parse_ipv4hdr for ipv4 frame header parsing.

Signed-off-by: Zhenyu Gao 
---
 datapath/flow.c | 230 
 1 file changed, 117 insertions(+), 113 deletions(-)

diff --git a/datapath/flow.c b/datapath/flow.c
index 2bc1ad0..0b35de6 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
  sizeof(struct icmphdr));
 }
 
+/**
+  * Parse ipv4 header from an Ethernet frame.
+  * Return ipv4 header length if successful, otherwise a negative errno value.
+  */
+static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
+{
+   int err;
+   struct iphdr *nh;
+   __be16 offset;
+
+   err = check_iphdr(skb);
+   if (unlikely(err))
+   return err;
+
+   nh = ip_hdr(skb);
+   key->ipv4.addr.src = nh->saddr;
+   key->ipv4.addr.dst = nh->daddr;
+
+   key->ip.proto = nh->protocol;
+   key->ip.tos = nh->tos;
+   key->ip.ttl = nh->ttl;
+
+   offset = nh->frag_off & htons(IP_OFFSET);
+   if (offset) {
+   key->ip.frag = OVS_FRAG_TYPE_LATER;
+   } else {
+   if (nh->frag_off & htons(IP_MF) ||
+   skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
+   key->ip.frag = OVS_FRAG_TYPE_FIRST;
+   } else {
+   key->ip.frag = OVS_FRAG_TYPE_NONE;
+   }
+   }
+   return ip_hdrlen(skb);
+}
+
+/**
+  * Parse ipv6 header from an Ethernet frame.
+  * Return ipv6 header length if successful, otherwise a negative errno value.
+  */
 static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
 {
unsigned int nh_ofs = skb_network_offset(skb);
@@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct 
sw_flow_key *key)
else
key->ip.frag = OVS_FRAG_TYPE_FIRST;
} else {
-   key->ip.frag = OVS_FRAG_TYPE_NONE;
+   if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
+   key->ip.frag = OVS_FRAG_TYPE_FIRST;
+   else
+   key->ip.frag = OVS_FRAG_TYPE_NONE;
}
 
/* Delayed handling of error in ipv6_skip_exthdr() as it
@@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct 
sw_flow_key *key)
key->eth.type = skb->protocol;
 
/* Network layer. */
-   if (key->eth.type == htons(ETH_P_IP)) {
-   struct iphdr *nh;
-   __be16 offset;
+   switch(key->eth.type) {
+   case htons(ETH_P_IP):
+   case htons(ETH_P_IPV6): {
+   int nh_len;
+   if (key->eth.type == htons(ETH_P_IP)) {
+   nh_len = parse_ipv4hdr(skb, key);
+   } else {
+   nh_len = parse_ipv6hdr(skb, key);
+   }
 
-   error = check_iphdr(skb);
-   if (unlikely(error)) {
-   memset(>ip, 0, sizeof(key->ip));
-   memset(>ipv4, 0, sizeof(key->ipv4));
-   if (error == -EINVAL) {
+   if (unlikely(nh_len < 0)) {
+   switch (nh_len) {
+   case -EINVAL:
+   memset(>ip, 0, sizeof(key->ip));
+   if (key->eth.type == htons(ETH_P_IP)) {
+   memset(>ipv4.addr, 0, 
sizeof(key->ipv4.addr));
+   } else {
+   memset(>ipv6.addr, 0, 
sizeof(key->ipv6.addr));
+   }
+   /* fall-through */
+   case -EPROTO:
skb->transport_header = skb->network_header;
error = 0;
+   break;
+   default:
+   error = nh_len;
}
return error;
}
 
-   nh = ip_hdr(skb);
-   key->ipv4.addr.src = nh->saddr;
-   key->ipv4.addr.dst = nh->daddr;
-
-   key->ip.proto = nh->protocol;
-   key->ip.tos = nh->tos;
-   key->ip.ttl = nh->ttl;
-
-   offset = nh->frag_off & htons(IP_OFFSET);
-   if (offset) {
-   key->ip.frag = OVS_FRAG_TYPE_LATER;
+   if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
return 0;
}
-   if (nh->frag_off & htons(IP_MF) ||
-   skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
-   key->ip.frag = OVS_FRAG_TYPE_FIRST;
-   else
-   key->ip.frag = OVS_FRAG_TYPE_NONE;
 
/*