| 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
| ||
|
Date: Thu, 06 Nov 2014 11:41:43 +0200
From: Eyal perry <eyalpe@....mellanox.co.il>
To: Ben Hutchings <ben@...adent.org.uk>,
Amir Vadai <amirv@...lanox.com>
CC: "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
Or Gerlitz <ogerlitz@...lanox.com>,
Eyal Perry <eyalpe@...lanox.com>,
Yevgeny Petrilin <yevgenyp@...lanox.com>
Subject: Re: [PATCH net-next 1/2] ethtool: Support for configurable RSS hash
function
On 11/5/2014 11:51 PM, Ben Hutchings wrote:
> On Wed, 2014-11-05 at 13:59 +0200, Amir Vadai wrote:
>> From: Eyal Perry <eyalpe@...lanox.com>
>>
>> This patch adds an RSS hash functions string-set, and two
>> ethtool-options for set/get current RSS hash function. User-kernel API is done
>> through the new hfunc mask field in the ethtool_rxfh struct. A bit set
>> in the hfunc is corresponding to an index in the string-set.
>>
>> Signed-off-by: Eyal Perry <eyalpe@...lanox.com>
>> Signed-off-by: Amir Vadai <amirv@...lanox.com>
>> ---
>> include/linux/ethtool.h | 28 ++++++++++++++++++++++++
>> include/uapi/linux/ethtool.h | 6 ++++-
>> net/core/ethtool.c | 52 ++++++++++++++++++++++++++++++--------------
>> 3 files changed, 69 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index c1a2d60..61003b1 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -59,6 +59,29 @@ enum ethtool_phys_id_state {
>> ETHTOOL_ID_OFF
>> };
>>
>> +enum {
>> + RSS_HASH_TOP_BIT, /* Configurable RSS hash function - Toeplitz */
>> + RSS_HASH_XOR_BIT, /* Configurable RSS hash function - Xor */
>> +
>> + /*
>> + * Add your fresh new hash function bits above and remember to update
>> + * rss_hash_func_strings[] below
>> + */
>> + RSS_HASH_FUNCS_COUNT
>> +};
>> +
>> +#define __RSS_HASH_BIT(bit) ((u32)1 << (bit))
>> +#define __RSS_HASH(name) __RSS_HASH_BIT(RSS_HASH_##name##_BIT)
>> +
>> +#define RSS_HASH_TOP __RSS_HASH(TOP)
>> +#define RSS_HASH_XOR __RSS_HASH(XOR)
>
> I think #define RSS_HASH_UNKNOWN 0 might also be useful.
>
> And I think all of these names should get an ETH_ prefix.
>
I'll add it in V1.
>> +static const char
>> +rss_hash_func_strings[RSS_HASH_FUNCS_COUNT][ETH_GSTRING_LEN] = {
>> + [RSS_HASH_TOP_BIT] = "toeplitz",
>> + [RSS_HASH_XOR_BIT] = "xor",
>> +};
>
> This belongs in net/core/ethtool.c.
>
I agree, will be fixed in V1.
>> struct net_device;
>>
>> /* Some generic methods drivers may use in their ethtool_ops */
>> @@ -158,6 +181,9 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
>> * Returns zero if not supported for this specific device.
>> * @get_rxfh_indir_size: Get the size of the RX flow hash indirection table.
>> * Returns zero if not supported for this specific device.
>> + * @get_rxfh_func: Get the hardware RX flow hash function.
>> + * @set_rxfh_func: Set the hardware RX flow hash function. Returns a negative
>> + * error code or zero.
>> * @get_rxfh: Get the contents of the RX flow hash indirection table and hash
>> * key.
>> * Will only be called if one or both of @get_rxfh_indir_size and
>> @@ -241,6 +267,8 @@ struct ethtool_ops {
>> int (*reset)(struct net_device *, u32 *);
>> u32 (*get_rxfh_key_size)(struct net_device *);
>> u32 (*get_rxfh_indir_size)(struct net_device *);
>> + u32 (*get_rxfh_func)(struct net_device *);
>> + int (*set_rxfh_func)(struct net_device *, u32);
>
> Why not another parameter to get_rxfh/set_rxfh? I know it's a pain to
> update all the implementations, but changing algorithm potentially
> changes the supported indirection table and key lengths. They have to
> be validated together.
>
OK, I'll do it for V1.
>> int (*get_rxfh)(struct net_device *, u32 *indir, u8 *key);
>> int (*set_rxfh)(struct net_device *, const u32 *indir,
>> const u8 *key);
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index eb2095b..eb91da4 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -534,6 +534,7 @@ struct ethtool_pauseparam {
>> * @ETH_SS_NTUPLE_FILTERS: Previously used with %ETHTOOL_GRXNTUPLE;
>> * now deprecated
>> * @ETH_SS_FEATURES: Device feature names
>> + * @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
>> */
>> enum ethtool_stringset {
>> ETH_SS_TEST = 0,
>> @@ -541,6 +542,7 @@ enum ethtool_stringset {
>> ETH_SS_PRIV_FLAGS,
>> ETH_SS_NTUPLE_FILTERS,
>> ETH_SS_FEATURES,
>> + ETH_SS_RSS_HASH_FUNCS,
>> };
>>
>> /**
>> @@ -900,7 +902,9 @@ struct ethtool_rxfh {
>> __u32 rss_context;
>> __u32 indir_size;
>> __u32 key_size;
>> - __u32 rsvd[2];
>> + __u8 hfunc;
>
> Missing kernel-doc. This needs to be very clear about what the valid
> values are.
>
I'll add it in V1.
>> + __u8 rsvd8[3];
>> + __u32 rsvd32;
>> __u32 rss_config[0];
>> };
>> #define ETH_RXFH_INDIR_NO_CHANGE 0xffffffff
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index 06dfb29..4791c17 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -185,6 +185,9 @@ static int __ethtool_get_sset_count(struct net_device *dev, int sset)
>> if (sset == ETH_SS_FEATURES)
>> return ARRAY_SIZE(netdev_features_strings);
>>
>> + if (sset == ETH_SS_RSS_HASH_FUNCS)
>> + return ARRAY_SIZE(rss_hash_func_strings);
>> +
>> if (ops->get_sset_count && ops->get_strings)
>> return ops->get_sset_count(dev, sset);
>> else
> [...]
>> @@ -760,32 +769,43 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>> const struct ethtool_ops *ops = dev->ethtool_ops;
>> struct ethtool_rxnfc rx_rings;
>> struct ethtool_rxfh rxfh;
>> - u32 dev_indir_size = 0, dev_key_size = 0, i;
>> + u32 dev_indir_size = 0, dev_key_size = 0, dev_hfunc = 0, i;
>> u32 *indir = NULL, indir_bytes = 0;
>> u8 *hkey = NULL;
>> u8 *rss_config;
>> u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
>>
>> - if (!(ops->get_rxfh_indir_size || ops->get_rxfh_key_size) ||
>> - !ops->get_rxnfc || !ops->set_rxfh)
>> + if (!(ops->get_rxfh_indir_size || ops->get_rxfh_key_size ||
>> + ops->get_rxfh_func) || !ops->get_rxnfc || !ops->set_rxfh)
>> return -EOPNOTSUPP;
>>
>> + if (ops->get_rxfh_func)
>> + dev_hfunc = ops->get_rxfh_func(dev);
>> if (ops->get_rxfh_indir_size)
>> dev_indir_size = ops->get_rxfh_indir_size(dev);
>> if (ops->get_rxfh_key_size)
>> dev_key_size = dev->ethtool_ops->get_rxfh_key_size(dev);
>> - if ((dev_key_size + dev_indir_size) == 0)
>> + if ((dev_key_size + dev_indir_size + dev_hfunc) == 0)
>> return -EOPNOTSUPP;
>>
>> if (copy_from_user(&rxfh, useraddr, sizeof(rxfh)))
>> return -EFAULT;
>>
>> /* Check that reserved fields are 0 for now */
>> - if (rxfh.rss_context || rxfh.rsvd[0] || rxfh.rsvd[1])
>> + if (rxfh.rss_context || rxfh.rsvd8[0] || rxfh.rsvd8[1] ||
>> + rxfh.rsvd8[2] || rxfh.rsvd32)
>> return -EINVAL;
>>
>> + if (rxfh.hfunc != dev_hfunc) {
>> + if (!ops->set_rxfh_func)
>> + return -EOPNOTSUPP;
>> + ret = ops->set_rxfh_func(dev, rxfh.hfunc);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> /* If either indir or hash key is valid, proceed further.
>> - * It is not valid to request that both be unchanged.
>> + * Must request at least one change: indir size, hash key or function.
>> */
>> if ((rxfh.indir_size &&
>> rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
>> @@ -793,7 +813,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>> (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
>> (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
>> rxfh.key_size == 0))
>> - return -EINVAL;
>> + return rxfh.hfunc ? 0 : -EINVAL;
>
> Shouldn't the condition be rxfh.hfunc != dev_hfunc ?
>
> Ben.
>
On current implementation it's not necessary since we have already
checked that above. However, according to your suggestion, after I'll
remove the set/get_rxfh_func() callbacks I wouldn't be able to check
that condition at this point of the code.
Best Regards,
Eyal.
--
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
Powered by blists - more mailing lists