netdev - Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS hash function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <545A2A99.9020001@dev.mellanox.co.il>
Date:	Wed, 05 Nov 2014 15:48:09 +0200
From:	Eyal perry <eyalpe@....mellanox.co.il>
To:	Or Gerlitz <ogerlitz@...lanox.com>,
	Eyal Perry <eyalpe@...lanox.com>
CC:	Amir Vadai <amirv@...lanox.com>,
	"David S. Miller" <davem@...emloft.net>,
	Ben Hutchings <ben@...adent.org.uk>, netdev@...r.kernel.org,
	Yevgeny Petrilin <yevgenyp@...lanox.com>
Subject: Re: [PATCH net-next 2/2] net/mlx4_en: Support for configurable RSS
 hash function

On 11/5/2014 3:30 PM, Or Gerlitz wrote:
> On 11/5/2014 3:05 PM, Eyal perry wrote:
>> On 11/5/2014 2:27 PM, Or Gerlitz wrote:
>>>> +
>>>> +static int mlx4_en_set_rxfh_func(struct net_device *dev, u32 hfunc)
>>>> +{
>>>> +    struct mlx4_en_priv *priv = netdev_priv(dev);
>>>> +    struct mlx4_en_dev *mdev = priv->mdev;
>>>> +    u32 prev_rss_hash_fn = priv->rss_hash_fn;
>>>> +    int err = 0;
>>>> +
>>>> +    if (!(hfunc & priv->rss_hash_fn_caps))
>>>> +        return -EINVAL;
>>>> +
>>>> +    priv->rss_hash_fn = hfunc;
>>> Seems that you are blindly setting here priv->rss_hash_fn to the
>>> function (xor or top) requested by the user w.o prior checking if the
>>> firmware actually supports that
>>> (e.g test it against priv->rss_hash_fn_caps, right?
>>>
>> That exactly what I do 3 lines above, priv->rss_hash_fn_caps is
>> derived from firmware capabilities.
> 
> got it, thanks.
> 
>>
>> [...]
>>
>>>> --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
>>>> @@ -1113,10 +1113,13 @@ int mlx4_en_config_rss_steer(struct
>>>> mlx4_en_priv *priv)
>>>>        }
>>>>          rss_context->flags = rss_mask;
>>>> -    rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>>> -    for (i = 0; i < 10; i++)
>>>> -        rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>>> -
>>>> +    if (priv->rss_hash_fn & MLX4_RSS_HASH_XOR) {
>>>> +        rss_context->hash_fn = MLX4_RSS_HASH_XOR;
>>>> +    } else {
>>>> +        rss_context->hash_fn = MLX4_RSS_HASH_TOP;
>>>> +        for (i = 0; i < 10; i++)
>>>> +            rss_context->rss_key[i] = cpu_to_be32(rsskey[i]);
>>>> +    }
>>> So this patch effectively changes the default from top to xor, right? is
>>> that intentional? if yes, spare few words on the change log.
>>>
>> No, in general the default shouldn't be affected by the patch (unless
>> there's a bug). The default value is set in mlx4_en_init_netdev() and it
>> will remain Toeplitz unless firmware is supports only XOR hash function.
>>
> 
> Oh, I see why I was confused and what I think need to be fixed. You are
> using both MLX4_RSS_HASH_TOP/XOR and RSS_HASH_TOP/XOR, any reason for that?
Yes, RSS_HASH_TOP/XOR are bits on a generic bitmask while the
MLX4_RSS_HASH_TOP/XOR are HW specific values.
> 
> Also, priv->rss_hash_fn should equal one value, right? so this code "if
> (priv->rss_hash_fn & something)" is confusing, should be "if
> (priv->rss_hash_fn == something)"
That's right, I'll fix it for V1.
Thanks.
> 
> Or.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html