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
| ||
|
Message-ID: <545B4257.8000001@dev.mellanox.co.il> 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