netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: Initialize entire flowi struct
@ 2011-08-31 16:05 David Ward
  2011-08-31 20:51 ` Julian Anastasov
  0 siblings, 1 reply; 14+ messages in thread
From: David Ward @ 2011-08-31 16:05 UTC (permalink / raw)
  To: netdev; +Cc: David Ward

The entire flowi struct needs to be initialized by afinfo->decode_session,
because flow_hash_code operates over the entire struct and may otherwise
return different hash values for what is intended to be the same key.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 net/ipv4/xfrm4_policy.c |    2 +-
 net/ipv6/xfrm6_policy.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index fc5368a..afce24d 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -114,7 +114,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)
 	u8 *xprth = skb_network_header(skb) + iph->ihl * 4;
 	struct flowi4 *fl4 = &fl->u.ip4;
 
-	memset(fl4, 0, sizeof(struct flowi4));
+	memset(fl, 0, sizeof(struct flowi));
 	fl4->flowi4_mark = skb->mark;
 
 	if (!ip_is_fragment(iph)) {
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index d879f7e..9088d38 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -129,7 +129,7 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
 	const unsigned char *nh = skb_network_header(skb);
 	u8 nexthdr = nh[IP6CB(skb)->nhoff];
 
-	memset(fl6, 0, sizeof(struct flowi6));
+	memset(fl, 0, sizeof(struct flowi));
 	fl6->flowi6_mark = skb->mark;
 
 	ipv6_addr_copy(&fl6->daddr, reverse ? &hdr->saddr : &hdr->daddr);
-- 
1.7.4.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: Initialize entire flowi struct
  2011-08-31 20:51 ` Julian Anastasov
@ 2011-08-31 20:47   ` David Miller
  2011-09-01 13:34   ` Ward, David - 0663 - MITLL
  1 sibling, 0 replies; 14+ messages in thread
From: David Miller @ 2011-08-31 20:47 UTC (permalink / raw)
  To: ja; +Cc: david.ward, netdev

From: Julian Anastasov <ja@ssi.bg>
Date: Wed, 31 Aug 2011 23:51:32 +0300 (EEST)

> 	Such change will cause problems for callers that
> use flowi4 in stack. Examples:
> 
> ip_route_me_harder
> icmp_route_lookup

Right.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: Initialize entire flowi struct
  2011-08-31 16:05 [PATCH] net: Initialize entire flowi struct David Ward
@ 2011-08-31 20:51 ` Julian Anastasov
  2011-08-31 20:47   ` David Miller
  2011-09-01 13:34   ` Ward, David - 0663 - MITLL
  0 siblings, 2 replies; 14+ messages in thread
From: Julian Anastasov @ 2011-08-31 20:51 UTC (permalink / raw)
  To: David Ward; +Cc: netdev


	Hello,

On Wed, 31 Aug 2011, David Ward wrote:

> The entire flowi struct needs to be initialized by afinfo->decode_session,
> because flow_hash_code operates over the entire struct and may otherwise
> return different hash values for what is intended to be the same key.

	Such change will cause problems for callers that
use flowi4 in stack. Examples:

ip_route_me_harder
icmp_route_lookup

	Not sure if adding size as parameter to flow_hash_code
is better approach. May be flow_cache_lookup needs to
determine size from family that can be used for flow_hash_code,
flow_key_compare and the memcpy(&fle->key, key, sizeof(*key))
after fle = kmem_cache_alloc(flow_cachep, GFP_ATOMIC). The
question is how to get size by family.

> Signed-off-by: David Ward <david.ward@ll.mit.edu>
> ---
>  net/ipv4/xfrm4_policy.c |    2 +-
>  net/ipv6/xfrm6_policy.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
> index fc5368a..afce24d 100644
> --- a/net/ipv4/xfrm4_policy.c
> +++ b/net/ipv4/xfrm4_policy.c
> @@ -114,7 +114,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)
>  	u8 *xprth = skb_network_header(skb) + iph->ihl * 4;
>  	struct flowi4 *fl4 = &fl->u.ip4;
>  
> -	memset(fl4, 0, sizeof(struct flowi4));
> +	memset(fl, 0, sizeof(struct flowi));
>  	fl4->flowi4_mark = skb->mark;
>  
>  	if (!ip_is_fragment(iph)) {
> diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
> index d879f7e..9088d38 100644
> --- a/net/ipv6/xfrm6_policy.c
> +++ b/net/ipv6/xfrm6_policy.c
> @@ -129,7 +129,7 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
>  	const unsigned char *nh = skb_network_header(skb);
>  	u8 nexthdr = nh[IP6CB(skb)->nhoff];
>  
> -	memset(fl6, 0, sizeof(struct flowi6));
> +	memset(fl, 0, sizeof(struct flowi));
>  	fl6->flowi6_mark = skb->mark;
>  
>  	ipv6_addr_copy(&fl6->daddr, reverse ? &hdr->saddr : &hdr->daddr);
> -- 
> 1.7.4.1

Regards

--
Julian Anastasov <ja@ssi.bg>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: Initialize entire flowi struct
  2011-08-31 20:51 ` Julian Anastasov
  2011-08-31 20:47   ` David Miller
@ 2011-09-01 13:34   ` Ward, David - 0663 - MITLL
  2011-09-03  7:27     ` Julian Anastasov
  1 sibling, 1 reply; 14+ messages in thread
From: Ward, David - 0663 - MITLL @ 2011-09-01 13:34 UTC (permalink / raw)
  To: Julian Anastasov; +Cc: David Miller, netdev

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]

Hi Julian,

On 08/31/2011 04:51 PM, Julian Anastasov wrote:
> On Wed, 31 Aug 2011, David Ward wrote
>> The entire flowi struct needs to be initialized by afinfo->decode_session,
>> because flow_hash_code operates over the entire struct and may otherwise
>> return different hash values for what is intended to be the same key.
> 	Such change will cause problems for callers that
> use flowi4 in stack. Examples:
>
> ip_route_me_harder
> icmp_route_lookup

Thanks for pointing this out.

> 	Not sure if adding size as parameter to flow_hash_code
> is better approach. May be flow_cache_lookup needs to
> determine size from family that can be used for flow_hash_code,
> flow_key_compare and the memcpy(&fle->key, key, sizeof(*key))
> after fle = kmem_cache_alloc(flow_cachep, GFP_ATOMIC).

Makes sense to me.  However should we just replace flow_key_compare with 
memcmp then, since the assumptions about constant size and alignment 
will no longer apply?  Or should there be a separate flow_key_compare 
function for each family, and have all of the flowi* structures become 
__attribute__((__aligned__(BITS_PER_LONG/8))) ?

David

> The question is how to get size by family.
>
>> Signed-off-by: David Ward<david.ward@ll.mit.edu>
>> ---
>>   net/ipv4/xfrm4_policy.c |    2 +-
>>   net/ipv6/xfrm6_policy.c |    2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
>> index fc5368a..afce24d 100644
>> --- a/net/ipv4/xfrm4_policy.c
>> +++ b/net/ipv4/xfrm4_policy.c
>> @@ -114,7 +114,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)
>>   	u8 *xprth = skb_network_header(skb) + iph->ihl * 4;
>>   	struct flowi4 *fl4 =&fl->u.ip4;
>>
>> -	memset(fl4, 0, sizeof(struct flowi4));
>> +	memset(fl, 0, sizeof(struct flowi));
>>   	fl4->flowi4_mark = skb->mark;
>>
>>   	if (!ip_is_fragment(iph)) {
>> diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
>> index d879f7e..9088d38 100644
>> --- a/net/ipv6/xfrm6_policy.c
>> +++ b/net/ipv6/xfrm6_policy.c
>> @@ -129,7 +129,7 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
>>   	const unsigned char *nh = skb_network_header(skb);
>>   	u8 nexthdr = nh[IP6CB(skb)->nhoff];
>>
>> -	memset(fl6, 0, sizeof(struct flowi6));
>> +	memset(fl, 0, sizeof(struct flowi));
>>   	fl6->flowi6_mark = skb->mark;
>>
>>   	ipv6_addr_copy(&fl6->daddr, reverse ?&hdr->saddr :&hdr->daddr);
>> -- 
>> 1.7.4.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4184 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: Initialize entire flowi struct
  2011-09-01 13:34   ` Ward, David - 0663 - MITLL
@ 2011-09-03  7:27     ` Julian Anastasov
  2011-09-04 13:07       ` [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs David Ward
                         ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Julian Anastasov @ 2011-09-03  7:27 UTC (permalink / raw)
  To: Ward, David - 0663 - MITLL; +Cc: David Miller, netdev


	Hello,

On Thu, 1 Sep 2011, Ward, David - 0663 - MITLL wrote:

> > 	Not sure if adding size as parameter to flow_hash_code
> > is better approach. May be flow_cache_lookup needs to
> > determine size from family that can be used for flow_hash_code,
> > flow_key_compare and the memcpy(&fle->key, key, sizeof(*key))
> > after fle = kmem_cache_alloc(flow_cachep, GFP_ATOMIC).
> 
> Makes sense to me.  However should we just replace flow_key_compare with
> memcmp then, since the assumptions about constant size and alignment will no
> longer apply?  Or should there be a separate flow_key_compare function for
> each family, and have all of the flowi* structures become
> __attribute__((__aligned__(BITS_PER_LONG/8))) ?

	I don't know this code well but I guess memcmp is
not preferred. IMHO, as the callers provide per-family
structures and we do not want to change that, these
structures must be aligned to long type as required by
flow_compare_t and jhash2 (at least u32) usage. The second
option is to use memcmp and jhash instead of jhash2 to
avoid such alignment but I guess other developers will
oppose it. More opinions are needed here.

Regards

--
Julian Anastasov <ja@ssi.bg>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs
  2011-09-03  7:27     ` Julian Anastasov
@ 2011-09-04 13:07       ` David Ward
  2011-09-04 13:07       ` [PATCH 1/2] net: Align AF-specific flowi structs to long David Ward
  2011-09-04 13:07       ` [PATCH 2/2] net: Handle different key sizes between address families in flow cache David Ward
  2 siblings, 0 replies; 14+ messages in thread
From: David Ward @ 2011-09-04 13:07 UTC (permalink / raw)
  To: netdev; +Cc: Julian Anastasov, David Ward

These fixes to the flow cache are needed with the conversion to AF-specific
flowi structs. They are written so as to avoid introducing AF-specific code
into net/core/flow.c.

Note that __xfrm_policy_check (in net/xfrm/xfrm_policy.c) still allocates a
struct flowi on the stack and passes it to flow_cache_lookup. My understanding
is that since this is on the stack, this will not be aligned, and therefore it
will cause problems with flow_hash_code and flow_key_compare. Is that correct?

Signed-off-by: David Ward <david.ward@ll.mit.edu>

David Ward (2):
  net: Align AF-specific flowi structs to long
  net: Handle different key sizes between address families in flow
    cache

 include/net/flow.h |   25 ++++++++++++++++++++++---
 net/core/flow.c    |   28 ++++++++++++++++------------
 2 files changed, 38 insertions(+), 15 deletions(-)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] net: Align AF-specific flowi structs to long
  2011-09-03  7:27     ` Julian Anastasov
  2011-09-04 13:07       ` [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs David Ward
@ 2011-09-04 13:07       ` David Ward
  2011-09-16 22:57         ` David Miller
  2011-09-04 13:07       ` [PATCH 2/2] net: Handle different key sizes between address families in flow cache David Ward
  2 siblings, 1 reply; 14+ messages in thread
From: David Ward @ 2011-09-04 13:07 UTC (permalink / raw)
  To: netdev; +Cc: Julian Anastasov, David Ward

AF-specific flowi structs are now passed to flow_key_compare, which must
also be aligned to a long.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 include/net/flow.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/flow.h b/include/net/flow.h
index 78113da..2ec377d 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -68,7 +68,7 @@ struct flowi4 {
 #define fl4_ipsec_spi		uli.spi
 #define fl4_mh_type		uli.mht.type
 #define fl4_gre_key		uli.gre_key
-};
+} __attribute__((__aligned__(BITS_PER_LONG/8)));
 
 static inline void flowi4_init_output(struct flowi4 *fl4, int oif,
 				      __u32 mark, __u8 tos, __u8 scope,
@@ -112,7 +112,7 @@ struct flowi6 {
 #define fl6_ipsec_spi		uli.spi
 #define fl6_mh_type		uli.mht.type
 #define fl6_gre_key		uli.gre_key
-};
+} __attribute__((__aligned__(BITS_PER_LONG/8)));
 
 struct flowidn {
 	struct flowi_common	__fl_common;
@@ -127,7 +127,7 @@ struct flowidn {
 	union flowi_uli		uli;
 #define fld_sport		uli.ports.sport
 #define fld_dport		uli.ports.dport
-};
+} __attribute__((__aligned__(BITS_PER_LONG/8)));
 
 struct flowi {
 	union {
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] net: Handle different key sizes between address families in flow cache
  2011-09-03  7:27     ` Julian Anastasov
  2011-09-04 13:07       ` [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs David Ward
  2011-09-04 13:07       ` [PATCH 1/2] net: Align AF-specific flowi structs to long David Ward
@ 2011-09-04 13:07       ` David Ward
  2011-09-04 15:34         ` Michał Mirosław
  2011-09-16 22:57         ` David Miller
  2 siblings, 2 replies; 14+ messages in thread
From: David Ward @ 2011-09-04 13:07 UTC (permalink / raw)
  To: netdev; +Cc: Julian Anastasov, David Ward

With the conversion of struct flowi to a union of AF-specific structs, some
operations on the flow cache need to account for the exact size of the key.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 include/net/flow.h |   19 +++++++++++++++++++
 net/core/flow.c    |   28 ++++++++++++++++------------
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/include/net/flow.h b/include/net/flow.h
index 2ec377d..99119f6 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -7,6 +7,7 @@
 #ifndef _NET_FLOW_H
 #define _NET_FLOW_H
 
+#include <linux/socket.h>
 #include <linux/in6.h>
 #include <linux/atomic.h>
 
@@ -161,6 +162,24 @@ static inline struct flowi *flowidn_to_flowi(struct flowidn *fldn)
 	return container_of(fldn, struct flowi, u.dn);
 }
 
+typedef unsigned long flow_compare_t;
+
+static inline size_t flowi_size(u16 family)
+{
+	switch (family) {
+	case AF_INET:
+		BUILD_BUG_ON(sizeof(struct flowi4) % sizeof(flow_compare_t));
+		return sizeof(struct flowi4);
+	case AF_INET6:
+		BUILD_BUG_ON(sizeof(struct flowi6) % sizeof(flow_compare_t));
+		return sizeof(struct flowi6);
+	case AF_DECnet:
+		BUILD_BUG_ON(sizeof(struct flowidn) % sizeof(flow_compare_t));
+		return sizeof(struct flowidn);
+	}
+	return 0;
+}
+
 #define FLOW_DIR_IN	0
 #define FLOW_DIR_OUT	1
 #define FLOW_DIR_FWD	2
diff --git a/net/core/flow.c b/net/core/flow.c
index bf32c33..1545445 100644
--- a/net/core/flow.c
+++ b/net/core/flow.c
@@ -172,26 +172,24 @@ static void flow_new_hash_rnd(struct flow_cache *fc,
 
 static u32 flow_hash_code(struct flow_cache *fc,
 			  struct flow_cache_percpu *fcp,
-			  const struct flowi *key)
+			  const struct flowi *key,
+			  size_t keysize)
 {
 	const u32 *k = (const u32 *) key;
 
-	return jhash2(k, (sizeof(*key) / sizeof(u32)), fcp->hash_rnd)
+	return jhash2(k, (keysize / sizeof(u32)), fcp->hash_rnd)
 		& (flow_cache_hash_size(fc) - 1);
 }
 
-typedef unsigned long flow_compare_t;
-
 /* I hear what you're saying, use memcmp.  But memcmp cannot make
  * important assumptions that we can here, such as alignment and
- * constant size.
+ * size.
  */
-static int flow_key_compare(const struct flowi *key1, const struct flowi *key2)
+static int flow_key_compare(const struct flowi *key1, const struct flowi *key2,
+			    size_t keysize)
 {
 	const flow_compare_t *k1, *k1_lim, *k2;
-	const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);
-
-	BUILD_BUG_ON(sizeof(struct flowi) % sizeof(flow_compare_t));
+	const int n_elem = keysize / sizeof(flow_compare_t);
 
 	k1 = (const flow_compare_t *) key1;
 	k1_lim = k1 + n_elem;
@@ -215,6 +213,7 @@ flow_cache_lookup(struct net *net, const struct flowi *key, u16 family, u8 dir,
 	struct flow_cache_entry *fle, *tfle;
 	struct hlist_node *entry;
 	struct flow_cache_object *flo;
+	size_t keysize;
 	unsigned int hash;
 
 	local_bh_disable();
@@ -222,6 +221,11 @@ flow_cache_lookup(struct net *net, const struct flowi *key, u16 family, u8 dir,
 
 	fle = NULL;
 	flo = NULL;
+
+	keysize = flowi_size(family);
+	if (!keysize)
+		goto nocache;
+
 	/* Packet really early in init?  Making flow_cache_init a
 	 * pre-smp initcall would solve this.  --RR */
 	if (!fcp->hash_table)
@@ -230,11 +234,11 @@ flow_cache_lookup(struct net *net, const struct flowi *key, u16 family, u8 dir,
 	if (fcp->hash_rnd_recalc)
 		flow_new_hash_rnd(fc, fcp);
 
-	hash = flow_hash_code(fc, fcp, key);
+	hash = flow_hash_code(fc, fcp, key, keysize);
 	hlist_for_each_entry(tfle, entry, &fcp->hash_table[hash], u.hlist) {
 		if (tfle->family == family &&
 		    tfle->dir == dir &&
-		    flow_key_compare(key, &tfle->key) == 0) {
+		    flow_key_compare(key, &tfle->key, keysize) == 0) {
 			fle = tfle;
 			break;
 		}
@@ -248,7 +252,7 @@ flow_cache_lookup(struct net *net, const struct flowi *key, u16 family, u8 dir,
 		if (fle) {
 			fle->family = family;
 			fle->dir = dir;
-			memcpy(&fle->key, key, sizeof(*key));
+			memcpy(&fle->key, key, keysize);
 			fle->object = NULL;
 			hlist_add_head(&fle->u.hlist, &fcp->hash_table[hash]);
 			fcp->hash_count++;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] net: Handle different key sizes between address families in flow cache
  2011-09-04 13:07       ` [PATCH 2/2] net: Handle different key sizes between address families in flow cache David Ward
@ 2011-09-04 15:34         ` Michał Mirosław
  2011-09-06  2:47           ` [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs David Ward
                             ` (2 more replies)
  2011-09-16 22:57         ` David Miller
  1 sibling, 3 replies; 14+ messages in thread
From: Michał Mirosław @ 2011-09-04 15:34 UTC (permalink / raw)
  To: David Ward; +Cc: netdev, Julian Anastasov

2011/9/4 David Ward <david.ward@ll.mit.edu>:
> With the conversion of struct flowi to a union of AF-specific structs, some
> operations on the flow cache need to account for the exact size of the key.
[...]
> --- a/include/net/flow.h
> +++ b/include/net/flow.h
[...]
> +typedef unsigned long flow_compare_t;
> +
> +static inline size_t flowi_size(u16 family)
> +{
> +       switch (family) {
> +       case AF_INET:
> +               BUILD_BUG_ON(sizeof(struct flowi4) % sizeof(flow_compare_t));
> +               return sizeof(struct flowi4);
> +       case AF_INET6:
> +               BUILD_BUG_ON(sizeof(struct flowi6) % sizeof(flow_compare_t));
> +               return sizeof(struct flowi6);
> +       case AF_DECnet:
> +               BUILD_BUG_ON(sizeof(struct flowidn) % sizeof(flow_compare_t));
> +               return sizeof(struct flowidn);
> +       }
> +       return 0;
> +}

Since most called user (flow_key_compare) uses returned value didided
by sizeof(flow_compare_t), you could just return divided value here
and save some shift operations by it.

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs
  2011-09-04 15:34         ` Michał Mirosław
@ 2011-09-06  2:47           ` David Ward
  2011-09-06  2:47           ` [PATCH 1/2] net: Align AF-specific flowi structs to long David Ward
  2011-09-06  2:47           ` [PATCH 2/2] net: Handle different key sizes between address families in flow cache David Ward
  2 siblings, 0 replies; 14+ messages in thread
From: David Ward @ 2011-09-06  2:47 UTC (permalink / raw)
  To: netdev; +Cc: Julian Anastasov, Michał Mirosław, David Ward

v2: Return the length of the flow key as a multiple of sizeof(flow_compare_t),
    to reduce the number of shift operations.

These fixes to the flow cache are needed with the conversion to AF-specific
flowi structs. They are written so as to avoid introducing AF-specific code
into net/core/flow.c.

Note that __xfrm_policy_check (in net/xfrm/xfrm_policy.c) still allocates a
struct flowi on the stack and passes it to flow_cache_lookup. My understanding
is that since this is on the stack, this will not be aligned, and therefore it
will cause problems with flow_hash_code and flow_key_compare. Is that correct?

Signed-off-by: David Ward <david.ward@ll.mit.edu>

David Ward (2):
  net: Align AF-specific flowi structs to long
  net: Handle different key sizes between address families in flow
    cache

 include/net/flow.h |   25 ++++++++++++++++++++++---
 net/core/flow.c    |   31 +++++++++++++++++--------------
 2 files changed, 39 insertions(+), 17 deletions(-)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] net: Align AF-specific flowi structs to long
  2011-09-04 15:34         ` Michał Mirosław
  2011-09-06  2:47           ` [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs David Ward
@ 2011-09-06  2:47           ` David Ward
  2011-09-06  2:47           ` [PATCH 2/2] net: Handle different key sizes between address families in flow cache David Ward
  2 siblings, 0 replies; 14+ messages in thread
From: David Ward @ 2011-09-06  2:47 UTC (permalink / raw)
  To: netdev; +Cc: Julian Anastasov, Michał Mirosław, David Ward

AF-specific flowi structs are now passed to flow_key_compare, which must
also be aligned to a long.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 include/net/flow.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/flow.h b/include/net/flow.h
index 78113da..2ec377d 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -68,7 +68,7 @@ struct flowi4 {
 #define fl4_ipsec_spi		uli.spi
 #define fl4_mh_type		uli.mht.type
 #define fl4_gre_key		uli.gre_key
-};
+} __attribute__((__aligned__(BITS_PER_LONG/8)));
 
 static inline void flowi4_init_output(struct flowi4 *fl4, int oif,
 				      __u32 mark, __u8 tos, __u8 scope,
@@ -112,7 +112,7 @@ struct flowi6 {
 #define fl6_ipsec_spi		uli.spi
 #define fl6_mh_type		uli.mht.type
 #define fl6_gre_key		uli.gre_key
-};
+} __attribute__((__aligned__(BITS_PER_LONG/8)));
 
 struct flowidn {
 	struct flowi_common	__fl_common;
@@ -127,7 +127,7 @@ struct flowidn {
 	union flowi_uli		uli;
 #define fld_sport		uli.ports.sport
 #define fld_dport		uli.ports.dport
-};
+} __attribute__((__aligned__(BITS_PER_LONG/8)));
 
 struct flowi {
 	union {
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] net: Handle different key sizes between address families in flow cache
  2011-09-04 15:34         ` Michał Mirosław
  2011-09-06  2:47           ` [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs David Ward
  2011-09-06  2:47           ` [PATCH 1/2] net: Align AF-specific flowi structs to long David Ward
@ 2011-09-06  2:47           ` David Ward
  2 siblings, 0 replies; 14+ messages in thread
From: David Ward @ 2011-09-06  2:47 UTC (permalink / raw)
  To: netdev; +Cc: Julian Anastasov, Michał Mirosław, David Ward

With the conversion of struct flowi to a union of AF-specific structs, some
operations on the flow cache need to account for the exact size of the key.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 include/net/flow.h |   19 +++++++++++++++++++
 net/core/flow.c    |   31 +++++++++++++++++--------------
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/include/net/flow.h b/include/net/flow.h
index 2ec377d..a094477 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -7,6 +7,7 @@
 #ifndef _NET_FLOW_H
 #define _NET_FLOW_H
 
+#include <linux/socket.h>
 #include <linux/in6.h>
 #include <linux/atomic.h>
 
@@ -161,6 +162,24 @@ static inline struct flowi *flowidn_to_flowi(struct flowidn *fldn)
 	return container_of(fldn, struct flowi, u.dn);
 }
 
+typedef unsigned long flow_compare_t;
+
+static inline size_t flow_key_size(u16 family)
+{
+	switch (family) {
+	case AF_INET:
+		BUILD_BUG_ON(sizeof(struct flowi4) % sizeof(flow_compare_t));
+		return sizeof(struct flowi4) / sizeof(flow_compare_t);
+	case AF_INET6:
+		BUILD_BUG_ON(sizeof(struct flowi6) % sizeof(flow_compare_t));
+		return sizeof(struct flowi6) / sizeof(flow_compare_t);
+	case AF_DECnet:
+		BUILD_BUG_ON(sizeof(struct flowidn) % sizeof(flow_compare_t));
+		return sizeof(struct flowidn) / sizeof(flow_compare_t);
+	}
+	return 0;
+}
+
 #define FLOW_DIR_IN	0
 #define FLOW_DIR_OUT	1
 #define FLOW_DIR_FWD	2
diff --git a/net/core/flow.c b/net/core/flow.c
index bf32c33..2d93a37 100644
--- a/net/core/flow.c
+++ b/net/core/flow.c
@@ -172,29 +172,26 @@ static void flow_new_hash_rnd(struct flow_cache *fc,
 
 static u32 flow_hash_code(struct flow_cache *fc,
 			  struct flow_cache_percpu *fcp,
-			  const struct flowi *key)
+			  const struct flowi *key,
+			  size_t keysize)
 {
 	const u32 *k = (const u32 *) key;
+	const u32 length = keysize * sizeof(flow_compare_t) / sizeof(u32); 
 
-	return jhash2(k, (sizeof(*key) / sizeof(u32)), fcp->hash_rnd)
+	return jhash2(k, length, fcp->hash_rnd)
 		& (flow_cache_hash_size(fc) - 1);
 }
 
-typedef unsigned long flow_compare_t;
-
 /* I hear what you're saying, use memcmp.  But memcmp cannot make
- * important assumptions that we can here, such as alignment and
- * constant size.
+ * important assumptions that we can here, such as alignment.
  */
-static int flow_key_compare(const struct flowi *key1, const struct flowi *key2)
+static int flow_key_compare(const struct flowi *key1, const struct flowi *key2,
+			    size_t keysize)
 {
 	const flow_compare_t *k1, *k1_lim, *k2;
-	const int n_elem = sizeof(struct flowi) / sizeof(flow_compare_t);
-
-	BUILD_BUG_ON(sizeof(struct flowi) % sizeof(flow_compare_t));
 
 	k1 = (const flow_compare_t *) key1;
-	k1_lim = k1 + n_elem;
+	k1_lim = k1 + keysize;
 
 	k2 = (const flow_compare_t *) key2;
 
@@ -215,6 +212,7 @@ flow_cache_lookup(struct net *net, const struct flowi *key, u16 family, u8 dir,
 	struct flow_cache_entry *fle, *tfle;
 	struct hlist_node *entry;
 	struct flow_cache_object *flo;
+	size_t keysize;
 	unsigned int hash;
 
 	local_bh_disable();
@@ -222,6 +220,11 @@ flow_cache_lookup(struct net *net, const struct flowi *key, u16 family, u8 dir,
 
 	fle = NULL;
 	flo = NULL;
+
+	keysize = flow_key_size(family);
+	if (!keysize)
+		goto nocache;
+
 	/* Packet really early in init?  Making flow_cache_init a
 	 * pre-smp initcall would solve this.  --RR */
 	if (!fcp->hash_table)
@@ -230,11 +233,11 @@ flow_cache_lookup(struct net *net, const struct flowi *key, u16 family, u8 dir,
 	if (fcp->hash_rnd_recalc)
 		flow_new_hash_rnd(fc, fcp);
 
-	hash = flow_hash_code(fc, fcp, key);
+	hash = flow_hash_code(fc, fcp, key, keysize);
 	hlist_for_each_entry(tfle, entry, &fcp->hash_table[hash], u.hlist) {
 		if (tfle->family == family &&
 		    tfle->dir == dir &&
-		    flow_key_compare(key, &tfle->key) == 0) {
+		    flow_key_compare(key, &tfle->key, keysize) == 0) {
 			fle = tfle;
 			break;
 		}
@@ -248,7 +251,7 @@ flow_cache_lookup(struct net *net, const struct flowi *key, u16 family, u8 dir,
 		if (fle) {
 			fle->family = family;
 			fle->dir = dir;
-			memcpy(&fle->key, key, sizeof(*key));
+			memcpy(&fle->key, key, keysize * sizeof(flow_compare_t));
 			fle->object = NULL;
 			hlist_add_head(&fle->u.hlist, &fcp->hash_table[hash]);
 			fcp->hash_count++;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] net: Align AF-specific flowi structs to long
  2011-09-04 13:07       ` [PATCH 1/2] net: Align AF-specific flowi structs to long David Ward
@ 2011-09-16 22:57         ` David Miller
  0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2011-09-16 22:57 UTC (permalink / raw)
  To: david.ward; +Cc: netdev, ja

From: David Ward <david.ward@ll.mit.edu>
Date: Sun, 4 Sep 2011 09:07:20 -0400

> AF-specific flowi structs are now passed to flow_key_compare, which must
> also be aligned to a long.
> 
> Signed-off-by: David Ward <david.ward@ll.mit.edu>

Applied.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] net: Handle different key sizes between address families in flow cache
  2011-09-04 13:07       ` [PATCH 2/2] net: Handle different key sizes between address families in flow cache David Ward
  2011-09-04 15:34         ` Michał Mirosław
@ 2011-09-16 22:57         ` David Miller
  1 sibling, 0 replies; 14+ messages in thread
From: David Miller @ 2011-09-16 22:57 UTC (permalink / raw)
  To: david.ward; +Cc: netdev, ja

From: David Ward <david.ward@ll.mit.edu>
Date: Sun, 4 Sep 2011 09:07:21 -0400

> With the conversion of struct flowi to a union of AF-specific structs, some
> operations on the flow cache need to account for the exact size of the key.
> 
> Signed-off-by: David Ward <david.ward@ll.mit.edu>

Applied.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2011-09-16 22:57 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-31 16:05 [PATCH] net: Initialize entire flowi struct David Ward
2011-08-31 20:51 ` Julian Anastasov
2011-08-31 20:47   ` David Miller
2011-09-01 13:34   ` Ward, David - 0663 - MITLL
2011-09-03  7:27     ` Julian Anastasov
2011-09-04 13:07       ` [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs David Ward
2011-09-04 13:07       ` [PATCH 1/2] net: Align AF-specific flowi structs to long David Ward
2011-09-16 22:57         ` David Miller
2011-09-04 13:07       ` [PATCH 2/2] net: Handle different key sizes between address families in flow cache David Ward
2011-09-04 15:34         ` Michał Mirosław
2011-09-06  2:47           ` [PATCH 0/2] Fixes to flow cache for AF-specifc flowi structs David Ward
2011-09-06  2:47           ` [PATCH 1/2] net: Align AF-specific flowi structs to long David Ward
2011-09-06  2:47           ` [PATCH 2/2] net: Handle different key sizes between address families in flow cache David Ward
2011-09-16 22:57         ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).