[PATCH RFC] net/mlx5_en: switch to Toeplitz RSS hash by default

XOR (MLX5_RX_HASH_FN_INVERTED_XOR8) gives only 8 bits.
It seems not enough for RFS. All other drivers use toeplitz.

Driver mlx4_en uses Toeplitz by default and warns if hash XOR is used
together with NETIF_F_RXHASH (enabled by default too): "Enabling both
XOR Hash function and RX Hashing can limit RPS functionality".

XOR is default in mlx5_en since commit 2be6967cdbc9
("net/mlx5e: Support ETH_RSS_HASH_XOR").

Hash function could be set via ethtool. But it would be nice to have
single standard for drivers or proper description why this one is special.

Signed-off-by: Konstantin Khlebnikov <redacted>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 5a7939e70190..def9fb5dcbff 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

@@ -4558,7 +4558,7 @@ void mlx5e_build_nic_params(struct mlx5_core_dev *mdev,
 	params->tx_min_inline_mode = mlx5e_params_calculate_tx_min_inline(mdev);
 
 	/* RSS */
-	params->rss_hfunc = ETH_RSS_HASH_XOR;
+	params->rss_hfunc = ETH_RSS_HASH_TOP;
 	netdev_rss_key_fill(params->toeplitz_hash_key, sizeof(params->toeplitz_hash_key));
 	mlx5e_build_default_indir_rqt(params->indirection_rqt,
 				      MLX5E_INDIR_RQT_SIZE, max_channels);

On 31/08/2018 2:29 PM, Konstantin Khlebnikov wrote:

XOR (MLX5_RX_HASH_FN_INVERTED_XOR8) gives only 8 bits.
It seems not enough for RFS. All other drivers use toeplitz.

Driver mlx4_en uses Toeplitz by default and warns if hash XOR is used
together with NETIF_F_RXHASH (enabled by default too): "Enabling both
XOR Hash function and RX Hashing can limit RPS functionality".

XOR is default in mlx5_en since commit 2be6967cdbc9
("net/mlx5e: Support ETH_RSS_HASH_XOR").

Hash function could be set via ethtool. But it would be nice to have
single standard for drivers or proper description why this one is special.

Signed-off-by: Konstantin Khlebnikov <redacted>
---

Hi Konstantin,

Thanks for the patch.

I understand the motivation.

This change affects the default out-of-the-box behavior and requires a 
full performance cycle. We'll run performance regression tomorrow, 
results should be ready by EOW.

I'll update.

Regards,
Tariq

On 02.09.2018 12:29, Tariq Toukan wrote:

On 31/08/2018 2:29 PM, Konstantin Khlebnikov wrote:

quoted

XOR (MLX5_RX_HASH_FN_INVERTED_XOR8) gives only 8 bits.
It seems not enough for RFS. All other drivers use toeplitz.

Driver mlx4_en uses Toeplitz by default and warns if hash XOR is used
together with NETIF_F_RXHASH (enabled by default too): "Enabling both
XOR Hash function and RX Hashing can limit RPS functionality".

XOR is default in mlx5_en since commit 2be6967cdbc9
("net/mlx5e: Support ETH_RSS_HASH_XOR").

Hash function could be set via ethtool. But it would be nice to have
single standard for drivers or proper description why this one is special.

Signed-off-by: Konstantin Khlebnikov <redacted>
---

Hi Konstantin,

Thanks for the patch.

I understand the motivation.

This change affects the default out-of-the-box behavior and requires a full performance cycle. We'll run performance regression tomorrow, 
results should be ready by EOW.
 > I'll update.

Ok, thank you.

The only mention I've found in your documentation
http://www.mellanox.com/related-docs/prod_software/Mellanox_EN_for_Linux_User_Manual_v4_4.pdf

is
---
1.1.10 RSS Support
1.1.10.1 RSS Hash Function
The device has the ability to use XOR as the RSS distribution function, instead of the default
Toplitz function.
The XOR function can be better distributed among driver's receive queues in small number of
streams, where it distributes each TCP/UDP stream to a different queue.
---

So Toeplitz is supposed to be default hash function for all versions of drivers and hardware.

Also XOR8 seems vulnerable for ddos - hash is predictable, no random\secret vector, only 8 bits.
So, it's easy to route all flows into one point. As we got it by accident.

Moreover, in kernel 4.4.y hash switch via ethtool is broken and does not work =)

Regards,
Tariq

On Sun, Sep 2, 2018 at 2:55 AM, Konstantin Khlebnikov
[off-list ref] wrote:

On 02.09.2018 12:29, Tariq Toukan wrote:

quoted

On 31/08/2018 2:29 PM, Konstantin Khlebnikov wrote:

quoted

XOR (MLX5_RX_HASH_FN_INVERTED_XOR8) gives only 8 bits.
It seems not enough for RFS. All other drivers use toeplitz.

Driver mlx4_en uses Toeplitz by default and warns if hash XOR is used
together with NETIF_F_RXHASH (enabled by default too): "Enabling both
XOR Hash function and RX Hashing can limit RPS functionality".

XOR is default in mlx5_en since commit 2be6967cdbc9
("net/mlx5e: Support ETH_RSS_HASH_XOR").

Hash function could be set via ethtool. But it would be nice to have
single standard for drivers or proper description why this one is
special.

Signed-off-by: Konstantin Khlebnikov <redacted>
---

Hi Konstantin,

Thanks for the patch.

I understand the motivation.

This change affects the default out-of-the-box behavior and requires a
full performance cycle. We'll run performance regression tomorrow, results
should be ready by EOW.
 > I'll update.

Ok, thank you.

The only mention I've found in your documentation
http://www.mellanox.com/related-docs/prod_software/Mellanox_EN_for_Linux_User_Manual_v4_4.pdf

is
---
1.1.10 RSS Support
1.1.10.1 RSS Hash Function
The device has the ability to use XOR as the RSS distribution function,
instead of the default
Toplitz function.
The XOR function can be better distributed among driver's receive queues in
small number of
streams, where it distributes each TCP/UDP stream to a different queue.
---

So Toeplitz is supposed to be default hash function for all versions of
drivers and hardware.

Also XOR8 seems vulnerable for ddos - hash is predictable, no random\secret
vector, only 8 bits.
So, it's easy to route all flows into one point. As we got it by accident.

Moreover, in kernel 4.4.y hash switch via ethtool is broken and does not
work =)

is it broken in mlx5 only or for the whole kernel ?

If it is mlx5 then this might be the reason:
commit 2d75b2bc8a8c0ce5567a6ecef52e194d117efe3f
net/mlx5e: Add ethtool RSS configuration options

was submitted to kernel 4.3

and an important fix for hash function change was submitted to 4.5:

commit bdfc028de1b3cd59490d5413a5c87b0fa50040c2
Author: Tariq Toukan [off-list ref]
Date:   Mon Feb 29 21:17:12 2016 +0200

    net/mlx5e: Fix ethtool RX hash func configuration change

    We should modify TIRs explicitly to apply the new RSS configuration.
    The light ndo close/open calls do not "refresh" them.

    Fixes: 2d75b2bc8a8c ('net/mlx5e: Add ethtool RSS configuration options')

quoted

Regards,
Tariq

On 06.09.2018 08:24, Saeed Mahameed wrote:

On Sun, Sep 2, 2018 at 2:55 AM, Konstantin Khlebnikov
[off-list ref] wrote:

quoted

On 02.09.2018 12:29, Tariq Toukan wrote:

quoted

On 31/08/2018 2:29 PM, Konstantin Khlebnikov wrote:

quoted

XOR (MLX5_RX_HASH_FN_INVERTED_XOR8) gives only 8 bits.
It seems not enough for RFS. All other drivers use toeplitz.

Driver mlx4_en uses Toeplitz by default and warns if hash XOR is used
together with NETIF_F_RXHASH (enabled by default too): "Enabling both
XOR Hash function and RX Hashing can limit RPS functionality".

XOR is default in mlx5_en since commit 2be6967cdbc9
("net/mlx5e: Support ETH_RSS_HASH_XOR").

Hash function could be set via ethtool. But it would be nice to have
single standard for drivers or proper description why this one is
special.

Signed-off-by: Konstantin Khlebnikov <redacted>
---

Hi Konstantin,

Thanks for the patch.

I understand the motivation.

This change affects the default out-of-the-box behavior and requires a
full performance cycle. We'll run performance regression tomorrow, results
should be ready by EOW.
  > I'll update.

Ok, thank you.

The only mention I've found in your documentation
http://www.mellanox.com/related-docs/prod_software/Mellanox_EN_for_Linux_User_Manual_v4_4.pdf

is
---
1.1.10 RSS Support
1.1.10.1 RSS Hash Function
The device has the ability to use XOR as the RSS distribution function,
instead of the default
Toplitz function.
The XOR function can be better distributed among driver's receive queues in
small number of
streams, where it distributes each TCP/UDP stream to a different queue.
---

So Toeplitz is supposed to be default hash function for all versions of
drivers and hardware.

Also XOR8 seems vulnerable for ddos - hash is predictable, no random\secret
vector, only 8 bits.
So, it's easy to route all flows into one point. As we got it by accident.

Moreover, in kernel 4.4.y hash switch via ethtool is broken and does not
work =)

is it broken in mlx5 only or for the whole kernel ?

In mlx5 only - interface works but makes no effect.

For now we have disabled RXHASH as workaround.

If it is mlx5 then this might be the reason:
commit 2d75b2bc8a8c0ce5567a6ecef52e194d117efe3f
net/mlx5e: Add ethtool RSS configuration options

was submitted to kernel 4.3

and an important fix for hash function change was submitted to 4.5:

commit bdfc028de1b3cd59490d5413a5c87b0fa50040c2
Author: Tariq Toukan [off-list ref]
Date:   Mon Feb 29 21:17:12 2016 +0200

     net/mlx5e: Fix ethtool RX hash func configuration change

     We should modify TIRs explicitly to apply the new RSS configuration.
     The light ndo close/open calls do not "refresh" them.

     Fixes: 2d75b2bc8a8c ('net/mlx5e: Add ethtool RSS configuration options')

I think so, but this patch also seems flawed and fixed in

commit a100ff3eef193d2d79daf98dcd97a54776ffeb78
Author: Gal Pressman [off-list ref]
Date:   Thu Jan 12 16:25:46 2017 +0200

     net/mlx5e: Fix update of hash function/key via ethtool

     Modifying TIR hash should change selected fields bitmask in addition to
     the function and key.

And maybe more, there a lot of progress since v4.4

quoted

quoted

Regards,
Tariq