[PATCH v4 net-next 09/11] net: Add IPv6 flow label to flow_keys

2015-05-28 Thread Tom Herbert
In flow_dissector set the flow label in flow_keys for IPv6. This also
removes the shortcircuiting of flow dissection when a non-zero label
is present, the flow label can be considered to provide additional
entropy for a hash.

Signed-off-by: Tom Herbert t...@herbertland.com
---
 include/net/flow_dissector.h |  4 +++-
 net/core/flow_dissector.c| 31 +++
 2 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 08480fb..14d8483 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -28,7 +28,8 @@ struct flow_dissector_key_basic {
 };
 
 struct flow_dissector_key_tags {
-   u32 vlan_id:12;
+   u32 vlan_id:12,
+   flow_label:20;
 };
 
 /**
@@ -111,6 +112,7 @@ enum flow_dissector_key_id {
FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs 
*/
FLOW_DISSECTOR_KEY_VLANID, /* struct flow_dissector_key_flow_tags */
+   FLOW_DISSECTOR_KEY_FLOW_LABEL, /* struct flow_dissector_key_flow_tags */
 
FLOW_DISSECTOR_KEY_MAX,
 };
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 5c66cb2..ba089d9 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -190,7 +190,7 @@ ip:
case htons(ETH_P_IPV6): {
const struct ipv6hdr *iph;
struct ipv6hdr _iph;
-   __be32 flow_label;
+   u32 flow_label;
 
 ipv6:
iph = __skb_header_pointer(skb, nhoff, sizeof(_iph), data, 
hlen, _iph);
@@ -210,30 +210,17 @@ ipv6:
 
memcpy(key_ipv6_addrs, iph-saddr, 
sizeof(*key_ipv6_addrs));
key_control-addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
-   goto flow_label;
}
-   break;
-flow_label:
+
flow_label = ip6_flowlabel(iph);
if (flow_label) {
-   /* Awesome, IPv6 packet has a flow label so we can
-* use that to represent the ports without any
-* further dissection.
-*/
-
-   key_basic-n_proto = proto;
-   key_basic-ip_proto = ip_proto;
-   key_control-thoff = (u16)nhoff;
-
if (skb_flow_dissector_uses_key(flow_dissector,
-   
FLOW_DISSECTOR_KEY_PORTS)) {
-   key_ports = 
skb_flow_dissector_target(flow_dissector,
- 
FLOW_DISSECTOR_KEY_PORTS,
- 
target_container);
-   key_ports-ports = flow_label;
+   FLOW_DISSECTOR_KEY_FLOW_LABEL)) {
+   key_tags = 
skb_flow_dissector_target(flow_dissector,
+
FLOW_DISSECTOR_KEY_FLOW_LABEL,
+
target_container);
+   key_tags-flow_label = ntohl(flow_label);
}
-
-   return true;
}
 
break;
@@ -659,6 +646,10 @@ static const struct flow_dissector_key 
flow_keys_dissector_keys[] = {
.key_id = FLOW_DISSECTOR_KEY_VLANID,
.offset = offsetof(struct flow_keys, tags),
},
+   {
+   .key_id = FLOW_DISSECTOR_KEY_FLOW_LABEL,
+   .offset = offsetof(struct flow_keys, tags),
+   },
 };
 
 static const struct flow_dissector_key flow_keys_buf_dissector_keys[] = {
-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 net-next 09/11] net: Add IPv6 flow label to flow_keys

2015-05-28 Thread Eric Dumazet
On Thu, 2015-05-28 at 11:19 -0700, Tom Herbert wrote:
 In flow_dissector set the flow label in flow_keys for IPv6. This also
 removes the shortcircuiting of flow dissection when a non-zero label
 is present, the flow label can be considered to provide additional
 entropy for a hash.
 
 Signed-off-by: Tom Herbert t...@herbertland.com
 ---
  include/net/flow_dissector.h |  4 +++-
  net/core/flow_dissector.c| 31 +++
  2 files changed, 14 insertions(+), 21 deletions(-)
 
 diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
 index 08480fb..14d8483 100644
 --- a/include/net/flow_dissector.h
 +++ b/include/net/flow_dissector.h
 @@ -28,7 +28,8 @@ struct flow_dissector_key_basic {
  };
  
  struct flow_dissector_key_tags {
 - u32 vlan_id:12;
 + u32 vlan_id:12,
 + flow_label:20;
  };
  
  /**
 @@ -111,6 +112,7 @@ enum flow_dissector_key_id {
   FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
   FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs 
 */
   FLOW_DISSECTOR_KEY_VLANID, /* struct flow_dissector_key_flow_tags */
 + FLOW_DISSECTOR_KEY_FLOW_LABEL, /* struct flow_dissector_key_flow_tags */
  
   FLOW_DISSECTOR_KEY_MAX,
  };
 diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
 index 5c66cb2..ba089d9 100644
 --- a/net/core/flow_dissector.c
 +++ b/net/core/flow_dissector.c
 @@ -190,7 +190,7 @@ ip:
   case htons(ETH_P_IPV6): {
   const struct ipv6hdr *iph;
   struct ipv6hdr _iph;
 - __be32 flow_label;
 + u32 flow_label;

You change flow_label from __be32 to u32.

  
  ipv6:
   iph = __skb_header_pointer(skb, nhoff, sizeof(_iph), data, 
 hlen, _iph);
 @@ -210,30 +210,17 @@ ipv6:
  
   memcpy(key_ipv6_addrs, iph-saddr, 
 sizeof(*key_ipv6_addrs));
   key_control-addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
 - goto flow_label;
   }
 - break;
 -flow_label:
 +
   flow_label = ip6_flowlabel(iph);

But ip6_flowlabel() returns a __be32. This should not please sparse.


   if (flow_label) {
 - /* Awesome, IPv6 packet has a flow label so we can
 -  * use that to represent the ports without any
 -  * further dissection.
 -  */
 -
 - key_basic-n_proto = proto;
 - key_basic-ip_proto = ip_proto;
 - key_control-thoff = (u16)nhoff;
 -
   if (skb_flow_dissector_uses_key(flow_dissector,
 - 
 FLOW_DISSECTOR_KEY_PORTS)) {
 - key_ports = 
 skb_flow_dissector_target(flow_dissector,
 -   
 FLOW_DISSECTOR_KEY_PORTS,
 -   
 target_container);
 - key_ports-ports = flow_label;
 + FLOW_DISSECTOR_KEY_FLOW_LABEL)) {
 + key_tags = 
 skb_flow_dissector_target(flow_dissector,
 +  
 FLOW_DISSECTOR_KEY_FLOW_LABEL,
 +  
 target_container);
 + key_tags-flow_label = ntohl(flow_label);

Then you call ntohl() on u32 variable. This should also complain.

Have you run sparse ?

make C=2 CF=-D__CHECK_ENDIAN__ net/core/flow_dissector.o

Thanks.


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 net-next 09/11] net: Add IPv6 flow label to flow_keys

2015-05-22 Thread Jiri Pirko
Fri, May 22, 2015 at 02:11:44AM CEST, t...@herbertland.com wrote:
In flow_dissector set the flow label in flow_keys for IPv6. This also
removes the shortcircuiting of flow dissection when a non-zero label
is present, the flow label can be considered to provide additional
entropy for a hash.

Signed-off-by: Tom Herbert t...@herbertland.com
---
 include/net/flow_dissector.h |  4 +++-
 net/core/flow_dissector.c| 37 +
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 08480fb..effe607 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -28,7 +28,8 @@ struct flow_dissector_key_basic {
 };
 
 struct flow_dissector_key_tags {
-  u32 vlan_id:12;
+  u32 vlan_id:12,
+  flow_label:20;
 };
 
 /**
@@ -111,6 +112,7 @@ enum flow_dissector_key_id {
   FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
   FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs 
 */
   FLOW_DISSECTOR_KEY_VLANID, /* struct flow_dissector_key_flow_tags */
+  FLOW_DISSECTOR_KEY_FLOW_LABEL, /* struct flow_dissector_key_flow_label 
*/


I think it makes sense to pair FLOW_DISSECTOR_KEY_* with
struct flow_dissector_key_*

How about to have FLOW_DISSECTOR_KEY_TAGS istead of VLANID and FLOW_LABEL?

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 net-next 09/11] net: Add IPv6 flow label to flow_keys

2015-05-22 Thread Tom Herbert
On Fri, May 22, 2015 at 8:22 AM, Jiri Pirko j...@resnulli.us wrote:
 Fri, May 22, 2015 at 05:14:21PM CEST, t...@herbertland.com wrote:
On Fri, May 22, 2015 at 1:14 AM, Jiri Pirko j...@resnulli.us wrote:
 Fri, May 22, 2015 at 02:11:44AM CEST, t...@herbertland.com wrote:
In flow_dissector set the flow label in flow_keys for IPv6. This also
removes the shortcircuiting of flow dissection when a non-zero label
is present, the flow label can be considered to provide additional
entropy for a hash.

Signed-off-by: Tom Herbert t...@herbertland.com
---
 include/net/flow_dissector.h |  4 +++-
 net/core/flow_dissector.c| 37 +
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 08480fb..effe607 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -28,7 +28,8 @@ struct flow_dissector_key_basic {
 };

 struct flow_dissector_key_tags {
-  u32 vlan_id:12;
+  u32 vlan_id:12,
+  flow_label:20;
 };

 /**
@@ -111,6 +112,7 @@ enum flow_dissector_key_id {
   FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs 
 */
   FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct 
 flow_dissector_key_tipc_addrs */
   FLOW_DISSECTOR_KEY_VLANID, /* struct flow_dissector_key_flow_tags */
+  FLOW_DISSECTOR_KEY_FLOW_LABEL, /* struct 
flow_dissector_key_flow_label */


 I think it makes sense to pair FLOW_DISSECTOR_KEY_* with
 struct flow_dissector_key_*

 How about to have FLOW_DISSECTOR_KEY_TAGS istead of VLANID and FLOW_LABEL?


I thought about that, but it doesn't really save anything to be less
explicit as we still need a conditional at each occurrence. If someone
is only looking for IPv6 flow label and nothing else they are able to
do that.

 So have 2 structs then.

We also use this structure in flow_keys and it's convenient to packet
both VLAN and and flow label into single structure in that case. So we
get precisely 24 or 48 bytes in flow_keys from skb_flow_dissector with
the trick os setting keys to same field. The comment for
FLOW_DISSECTOR_KEY_FLOW_LABEL does need to be fixed.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 net-next 09/11] net: Add IPv6 flow label to flow_keys

2015-05-22 Thread Tom Herbert
On Fri, May 22, 2015 at 1:14 AM, Jiri Pirko j...@resnulli.us wrote:
 Fri, May 22, 2015 at 02:11:44AM CEST, t...@herbertland.com wrote:
In flow_dissector set the flow label in flow_keys for IPv6. This also
removes the shortcircuiting of flow dissection when a non-zero label
is present, the flow label can be considered to provide additional
entropy for a hash.

Signed-off-by: Tom Herbert t...@herbertland.com
---
 include/net/flow_dissector.h |  4 +++-
 net/core/flow_dissector.c| 37 +
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 08480fb..effe607 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -28,7 +28,8 @@ struct flow_dissector_key_basic {
 };

 struct flow_dissector_key_tags {
-  u32 vlan_id:12;
+  u32 vlan_id:12,
+  flow_label:20;
 };

 /**
@@ -111,6 +112,7 @@ enum flow_dissector_key_id {
   FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
   FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs 
 */
   FLOW_DISSECTOR_KEY_VLANID, /* struct flow_dissector_key_flow_tags */
+  FLOW_DISSECTOR_KEY_FLOW_LABEL, /* struct flow_dissector_key_flow_label 
*/


 I think it makes sense to pair FLOW_DISSECTOR_KEY_* with
 struct flow_dissector_key_*

 How about to have FLOW_DISSECTOR_KEY_TAGS istead of VLANID and FLOW_LABEL?


I thought about that, but it doesn't really save anything to be less
explicit as we still need a conditional at each occurrence. If someone
is only looking for IPv6 flow label and nothing else they are able to
do that.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 net-next 09/11] net: Add IPv6 flow label to flow_keys

2015-05-22 Thread Jiri Pirko
Fri, May 22, 2015 at 05:14:21PM CEST, t...@herbertland.com wrote:
On Fri, May 22, 2015 at 1:14 AM, Jiri Pirko j...@resnulli.us wrote:
 Fri, May 22, 2015 at 02:11:44AM CEST, t...@herbertland.com wrote:
In flow_dissector set the flow label in flow_keys for IPv6. This also
removes the shortcircuiting of flow dissection when a non-zero label
is present, the flow label can be considered to provide additional
entropy for a hash.

Signed-off-by: Tom Herbert t...@herbertland.com
---
 include/net/flow_dissector.h |  4 +++-
 net/core/flow_dissector.c| 37 +
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 08480fb..effe607 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -28,7 +28,8 @@ struct flow_dissector_key_basic {
 };

 struct flow_dissector_key_tags {
-  u32 vlan_id:12;
+  u32 vlan_id:12,
+  flow_label:20;
 };

 /**
@@ -111,6 +112,7 @@ enum flow_dissector_key_id {
   FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs 
 */
   FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct 
 flow_dissector_key_tipc_addrs */
   FLOW_DISSECTOR_KEY_VLANID, /* struct flow_dissector_key_flow_tags */
+  FLOW_DISSECTOR_KEY_FLOW_LABEL, /* struct 
flow_dissector_key_flow_label */


 I think it makes sense to pair FLOW_DISSECTOR_KEY_* with
 struct flow_dissector_key_*

 How about to have FLOW_DISSECTOR_KEY_TAGS istead of VLANID and FLOW_LABEL?


I thought about that, but it doesn't really save anything to be less
explicit as we still need a conditional at each occurrence. If someone
is only looking for IPv6 flow label and nothing else they are able to
do that.

So have 2 structs then.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v4 net-next 09/11] net: Add IPv6 flow label to flow_keys

2015-05-21 Thread Tom Herbert
In flow_dissector set the flow label in flow_keys for IPv6. This also
removes the shortcircuiting of flow dissection when a non-zero label
is present, the flow label can be considered to provide additional
entropy for a hash.

Signed-off-by: Tom Herbert t...@herbertland.com
---
 include/net/flow_dissector.h |  4 +++-
 net/core/flow_dissector.c| 37 +
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 08480fb..effe607 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -28,7 +28,8 @@ struct flow_dissector_key_basic {
 };
 
 struct flow_dissector_key_tags {
-   u32 vlan_id:12;
+   u32 vlan_id:12,
+   flow_label:20;
 };
 
 /**
@@ -111,6 +112,7 @@ enum flow_dissector_key_id {
FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs 
*/
FLOW_DISSECTOR_KEY_VLANID, /* struct flow_dissector_key_flow_tags */
+   FLOW_DISSECTOR_KEY_FLOW_LABEL, /* struct flow_dissector_key_flow_label 
*/
 
FLOW_DISSECTOR_KEY_MAX,
 };
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 3512366..2fba492 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -190,7 +190,7 @@ ip:
case htons(ETH_P_IPV6): {
const struct ipv6hdr *iph;
struct ipv6hdr _iph;
-   __be32 flow_label;
+   u32 flow_label;
 
 ipv6:
iph = __skb_header_pointer(skb, nhoff, sizeof(_iph), data, 
hlen, _iph);
@@ -210,30 +210,15 @@ ipv6:
 
memcpy(key_ipv6_addrs, iph-saddr, 
sizeof(*key_ipv6_addrs));
key_control-addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
-   goto flow_label;
}
-   break;
-flow_label:
-   flow_label = ip6_flowlabel(iph);
-   if (flow_label) {
-   /* Awesome, IPv6 packet has a flow label so we can
-* use that to represent the ports without any
-* further dissection.
-*/
-
-   key_basic-n_proto = proto;
-   key_basic-ip_proto = ip_proto;
-   key_control-thoff = (u16)nhoff;
-
-   if (!skb_flow_dissector_uses_key(flow_dissector,
-
FLOW_DISSECTOR_KEY_PORTS))
-   break;
-   key_ports = skb_flow_dissector_target(flow_dissector,
- 
FLOW_DISSECTOR_KEY_PORTS,
- target_container);
-   key_ports-ports = flow_label;
-
-   return true;
+
+   flow_label = ntohl(ip6_flowlabel(iph));
+   if (flow_label  skb_flow_dissector_uses_key(flow_dissector,
+   FLOW_DISSECTOR_KEY_FLOW_LABEL)) {
+   key_tags = skb_flow_dissector_target(flow_dissector,
+   FLOW_DISSECTOR_KEY_FLOW_LABEL, target_container);
+
+   key_tags-flow_label = flow_label;
}
 
break;
@@ -659,6 +644,10 @@ static const struct flow_dissector_key 
flow_keys_dissector_keys[] = {
.key_id = FLOW_DISSECTOR_KEY_VLANID,
.offset = offsetof(struct flow_keys, tags),
},
+   {
+   .key_id = FLOW_DISSECTOR_KEY_FLOW_LABEL,
+   .offset = offsetof(struct flow_keys, tags),
+   },
 };
 
 static const struct flow_dissector_key flow_keys_buf_dissector_keys[] = {
-- 
1.8.1

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html