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

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

[Konstantin Khlebnikov]

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 <khlebnikov@yandex-team.ru>
---
 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);

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

[Tariq Toukan]

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 <khlebnikov@yandex-team.ru>
> ---

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

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

[Konstantin Khlebnikov]

On 02.09.2018 12:29, Tariq Toukan wrote:
> 
> 
> 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 <khlebnikov@yandex-team.ru>
>> ---
> 
> 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

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

[Saeed Mahameed]

On Sun, Sep 2, 2018 at 2:55 AM, Konstantin Khlebnikov
<khlebnikov@yandex-team.ru> wrote:
> On 02.09.2018 12:29, Tariq Toukan wrote:
>>
>>
>>
>> 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 <khlebnikov@yandex-team.ru>
>>> ---
>>
>>
>> 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 <tariqt@mellanox.com>
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')


>>
>> Regards,
>> Tariq

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

[Konstantin Khlebnikov]

On 06.09.2018 08:24, Saeed Mahameed wrote:
> On Sun, Sep 2, 2018 at 2:55 AM, Konstantin Khlebnikov
> <khlebnikov@yandex-team.ru> wrote:
>> On 02.09.2018 12:29, Tariq Toukan wrote:
>>>
>>>
>>>
>>> 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 <khlebnikov@yandex-team.ru>
>>>> ---
>>>
>>>
>>> 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 <tariqt@mellanox.com>
> 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 <galp@mellanox.com>
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

> 
>>>
>>> Regards,
>>> Tariq