[PATCH net-next] flow_dissector: remove __flow_hash_consistentify

[PATCH net-next] flow_dissector: remove __flow_hash_consistentify

[Tom Herbert]

The intent of this function was to produce a consistent hash for both
directions of a flow. However, since we added more inputs to the flow
hashing (IPv6 flow labels for instance) in a lot of cases we won't get
the same hash computed for each direction anyway. Also, there is no
defined correlation between the hashes computed in each direction of a
flow.

This patch removes the function since it is not providing significant
value and is expensive to be called for every packet. If there are
ever users of the flow_hash_from_keys that did require consistency
they can swap addresses and ports as needed in the flow_keys before
calling flow_hash_from_keys.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 net/core/flow_dissector.c | 35 -----------------------------------
 1 file changed, 35 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 2a834c6..9c9236b 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -500,45 +500,10 @@ __be32 flow_get_u32_dst(const struct flow_keys *flow)
 }
 EXPORT_SYMBOL(flow_get_u32_dst);
 
-static inline void __flow_hash_consistentify(struct flow_keys *keys)
-{
-	int addr_diff, i;
-
-	switch (keys->control.addr_type) {
-	case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
-		addr_diff = (__force u32)keys->addrs.v4addrs.dst -
-			    (__force u32)keys->addrs.v4addrs.src;
-		if ((addr_diff < 0) ||
-		    (addr_diff == 0 &&
-		     ((__force u16)keys->ports.dst <
-		      (__force u16)keys->ports.src))) {
-			swap(keys->addrs.v4addrs.src, keys->addrs.v4addrs.dst);
-			swap(keys->ports.src, keys->ports.dst);
-		}
-		break;
-	case FLOW_DISSECTOR_KEY_IPV6_ADDRS:
-		addr_diff = memcmp(&keys->addrs.v6addrs.dst,
-				   &keys->addrs.v6addrs.src,
-				   sizeof(keys->addrs.v6addrs.dst));
-		if ((addr_diff < 0) ||
-		    (addr_diff == 0 &&
-		     ((__force u16)keys->ports.dst <
-		      (__force u16)keys->ports.src))) {
-			for (i = 0; i < 4; i++)
-				swap(keys->addrs.v6addrs.src.s6_addr32[i],
-				     keys->addrs.v6addrs.dst.s6_addr32[i]);
-			swap(keys->ports.src, keys->ports.dst);
-		}
-		break;
-	}
-}
-
 static inline u32 __flow_hash_from_keys(struct flow_keys *keys, u32 keyval)
 {
 	u32 hash;
 
-	__flow_hash_consistentify(keys);
-
 	hash = __flow_hash_words((u32 *)flow_keys_hash_start(keys),
 				 flow_keys_hash_length(keys), keyval);
 	if (!hash)
-- 
1.8.1

Re: [PATCH net-next] flow_dissector: remove __flow_hash_consistentify

[Eric Dumazet]

On Wed, 2015-07-29 at 13:49 -0700, Tom Herbert wrote:
> The intent of this function was to produce a consistent hash for both
> directions of a flow. However, since we added more inputs to the flow
> hashing (IPv6 flow labels for instance) in a lot of cases we won't get
> the same hash computed for each direction anyway. Also, there is no
> defined correlation between the hashes computed in each direction of a
> flow.
> 
> This patch removes the function since it is not providing significant
> value and is expensive to be called for every packet. If there are
> ever users of the flow_hash_from_keys that did require consistency
> they can swap addresses and ports as needed in the flow_keys before
> calling flow_hash_from_keys.

Have you tested this change with conntracking and RPS enabled ?

This was whole point from commit b249dcb82d327e41

I guess difference is even bigger today after removal of central
conntracking lock.

Re: [PATCH net-next] flow_dissector: remove __flow_hash_consistentify

[Tom Herbert]

On Wed, Jul 29, 2015 at 2:19 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2015-07-29 at 13:49 -0700, Tom Herbert wrote:
>> The intent of this function was to produce a consistent hash for both
>> directions of a flow. However, since we added more inputs to the flow
>> hashing (IPv6 flow labels for instance) in a lot of cases we won't get
>> the same hash computed for each direction anyway. Also, there is no
>> defined correlation between the hashes computed in each direction of a
>> flow.
>>
>> This patch removes the function since it is not providing significant
>> value and is expensive to be called for every packet. If there are
>> ever users of the flow_hash_from_keys that did require consistency
>> they can swap addresses and ports as needed in the flow_keys before
>> calling flow_hash_from_keys.
>
> Have you tested this change with conntracking and RPS enabled ?
>
> This was whole point from commit b249dcb82d327e41
>
> I guess difference is even bigger today after removal of central
> conntracking lock.
>
Hi Eric,

So the scenario you're thinking is conntrack in the forwarding path,
RPS enabled (RSS not relevant), no hash from device, no IPv6 flow
labels or any other asymmetric inputs into the flow hash? I can look
at that, but it does make me wonder if maybe conntrack should set RFS
for both sides to avoid any issue with asymmetric hashes. With more
IPv6 and flow labels (which we will enable by default), asymmetric
hashes will likely become the norm.

Thanks,
Tom


>
>

Re: [PATCH net-next] flow_dissector: remove __flow_hash_consistentify

[Eric Dumazet]

On Wed, 2015-07-29 at 14:47 -0700, Tom Herbert wrote:

> Hi Eric,
> 
> So the scenario you're thinking is conntrack in the forwarding path,
> RPS enabled (RSS not relevant), no hash from device, no IPv6 flow
> labels or any other asymmetric inputs into the flow hash? I can look
> at that, but it does make me wonder if maybe conntrack should set RFS
> for both sides to avoid any issue with asymmetric hashes. With more
> IPv6 and flow labels (which we will enable by default), asymmetric
> hashes will likely become the norm.

Yes, but as long as the hash was done in software in our stack we could
use flow dissection and this swap() thing, regardless of the hashes
computed by the NIC or derived from IPv6 flow label.

RFS wont fly here, as DDOS traffic will need more cache misses.

Re: [PATCH net-next] flow_dissector: remove __flow_hash_consistentify

[Tom Herbert]

On Wed, Jul 29, 2015 at 3:15 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2015-07-29 at 14:47 -0700, Tom Herbert wrote:
>
>> Hi Eric,
>>
>> So the scenario you're thinking is conntrack in the forwarding path,
>> RPS enabled (RSS not relevant), no hash from device, no IPv6 flow
>> labels or any other asymmetric inputs into the flow hash? I can look
>> at that, but it does make me wonder if maybe conntrack should set RFS
>> for both sides to avoid any issue with asymmetric hashes. With more
>> IPv6 and flow labels (which we will enable by default), asymmetric
>> hashes will likely become the norm.
>
> Yes, but as long as the hash was done in software in our stack we could
> use flow dissection and this swap() thing, regardless of the hashes
> computed by the NIC or derived from IPv6 flow label.
>
The IPv6 flow label is now taken as input to the software hash (along
with VLAN id, GRE keyid, MPLS label, etc.). Packets encapsulated in
UDP also cause asymmetric hashes since the source port is used for
entropy. We have no way to predict what a remote host will put into
these fields, so our only recourse would be not include them in the
hash if symmetric hashes are really a requirement.

> RFS wont fly here, as DDOS traffic will need more cache misses.
>
>

Re: [PATCH net-next] flow_dissector: remove __flow_hash_consistentify

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Wed, 29 Jul 2015 13:49:03 -0700

> The intent of this function was to produce a consistent hash for both
> directions of a flow. However, since we added more inputs to the flow
> hashing (IPv6 flow labels for instance) in a lot of cases we won't get
> the same hash computed for each direction anyway. Also, there is no
> defined correlation between the hashes computed in each direction of a
> flow.
> 
> This patch removes the function since it is not providing significant
> value and is expensive to be called for every packet. If there are
> ever users of the flow_hash_from_keys that did require consistency
> they can swap addresses and ports as needed in the flow_keys before
> calling flow_hash_from_keys.
> 
> Signed-off-by: Tom Herbert <tom@herbertland.com>

The conntrack issue needs to be sorted out before I can consider this
change seriously.