Re: [PATCH net-next] flow_dissector: remove __flow_hash_consistentify
From: Tom Herbert t...@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 t...@herbertland.com The conntrack issue needs to be sorted out before I can consider this change seriously. -- 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 net-next] flow_dissector: remove __flow_hash_consistentify
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. -- 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 net-next] flow_dissector: remove __flow_hash_consistentify
On Wed, Jul 29, 2015 at 3:15 PM, Eric Dumazet eric.duma...@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. -- 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 net-next] flow_dissector: remove __flow_hash_consistentify
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. -- 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 net-next] flow_dissector: remove __flow_hash_consistentify
On Wed, Jul 29, 2015 at 2:19 PM, Eric Dumazet eric.duma...@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 -- 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
