netdev - Re: [PATCH ethtool 5/5] ethtool: 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: <1428202547.11260.160.camel@decadent.org.uk>
Date:	Sun, 05 Apr 2015 03:55:47 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Amir Vadai <amirv@...lanox.com>
Cc:	netdev@...r.kernel.org, Or Gerlitz <ogerlitz@...lanox.com>,
	Yevgeny Petrilin <yevgenyp@...lanox.com>,
	Saeed Mahameed <saeedm@...lanox.com>,
	Eyal Perry <eyalpe@...lanox.com>
Subject: Re: [PATCH ethtool 5/5] ethtool: Support for configurable RSS hash
 function

On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
> From: Eyal Perry <eyalpe@...lanox.com>
> 
> This ethtool patch adds support to set and get the current RSS hash
> function for the device through through the new hfunc mask field in the
> ethtool_rxfh struct. Kernel supported hash function names are queried
> with ETHTOOL_GSTRINGS - each string is corresponding with a bit in hfunc
> mask according to its index in the string-set.
> 
> Signed-off-by: Eyal Perry <eyalpe@...lanox.com>
> Signed-off-by: Amir Vadai <amirv@...lanox.com>
> ---
>  ethtool.8.in |  7 +++++++
>  ethtool.c    | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 58 insertions(+), 5 deletions(-)
> 
> diff --git a/ethtool.8.in b/ethtool.8.in
> index ae56293..bdc77e0 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -297,6 +297,8 @@ ethtool \- query or control network driver and hardware settings
>  .BI weight\  W0
>  .IR W1
>  .RB ...\ ]
> +.RB [ hfunc
> +.IR FUNC ]
>  .HP
>  .B ethtool \-f|\-\-flash
>  .I devname file
> @@ -796,6 +798,11 @@ Sets RSS hash key of the specified network device. RSS hash key should be of dev
>  Hash key format must be in xx:yy:zz:aa:bb:cc format meaning both the nibbles of a byte should be mentioned
>  even if a nibble is zero.
>  .TP
> +.BI hfunc
> +Sets RSS hash function of the specified network device. Requested hash function
> +should be supported by the kernel and the device.

I think the second sentence is unneeded - obviously requesting an
unsupported hash function will fail.

>  List of RSS hash functions

'A list of ...'

> +which kernel supports is shown as a part of the --show-rxfh comand output.

'... which the device supports ...' (since this depends on the hardware
and driver as well as the kernel).

> +.TP
>  .BI equal\  N
>  Sets the receive flow hash indirection table to spread flows evenly
>  between the first \fIN\fR receive queues.
> diff --git a/ethtool.c b/ethtool.c
> index c2f4164..c584333 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -3176,10 +3176,11 @@ static int do_grxfh(struct cmd_context *ctx)
>  {
>  	struct ethtool_rxfh rss_head = {0};
>  	struct ethtool_rxnfc ring_count;
> +	struct ethtool_gstrings *hfuncs;
>  	struct ethtool_rxfh *rss;
>  	u32 i, indir_bytes;
>  	char *hkey;
> -	int err;
> +	int err, cur_len, max_len = 0;
>  
>  	ring_count.cmd = ETHTOOL_GRXRINGS;
>  	err = send_ioctl(ctx, &ring_count);
> @@ -3231,6 +3232,23 @@ static int do_grxfh(struct cmd_context *ctx)
>  			printf("%02x:", (u8) hkey[i]);
>  	}
>  
> +	printf("RSS hash function:\n");
> +	if (!rss->hfunc) {
> +		printf("    Operation not supported\n");
> +		goto out;
> +	}
> +	hfuncs = get_stringset(ctx, ETH_SS_RSS_HASH_FUNCS, 0, 1);
> +	for (i = 0; i < hfuncs->len; i++) {
> +		cur_len = strlen((const char *)hfuncs->data +
> +				 i * ETH_GSTRING_LEN);
> +		if (cur_len > max_len)
> +			max_len = cur_len;
> +	}
> +	for (i = 0; i < hfuncs->len; i++)
> +		printf("    %-*s: %s\n", max_len,
> +		       (const char *)hfuncs->data + i * ETH_GSTRING_LEN,
> +		       (rss->hfunc & (1 << i)) ? "on" : "off");

I'm puzzled by this.  Do you remember why the hash functions were given
flag numbers rather than serial numbers?  Would it ever make sense to
have multiple hash functions enabled?

If only one hash function can be enabled (which seems to be true at
least for current drivers) then this only needs to print the one hash
function that is enabled.  (But then, the user needs to be able to list
the available hash functions somehow.)

If multiple hash functions can be enabled, then we should support that
in do_srxfh(), don't we?

> +out:
>  	free(rss);
>  	return 0;
>  }
[...]
> @@ -3410,6 +3454,7 @@ static int do_srxfh(struct cmd_context *ctx)
>  	rss->cmd = ETHTOOL_SRSSH;
>  	rss->indir_size = rss_head.indir_size;
>  	rss->key_size = rss_head.key_size;
> +	rss->hfunc = req_hfunc ? req_hfunc : 0;

That's a complicated way to write '= req_hfunc;'.

Ben.

>  	if (fill_indir_table(&rss->indir_size, rss->rss_config, rxfhindir_equal,
>  			     rxfhindir_weight, num_weights)) {
> @@ -4119,7 +4164,8 @@ static const struct option {
>  	{ "-X|--set-rxfh-indir|--rxfh", 1, do_srxfh,
>  	  "Set Rx flow hash indirection and/or hash key",
>  	  "		[ equal N | weight W0 W1 ... ]\n"
> -	  "		[ hkey %x:%x:%x:%x:%x:.... ]\n" },
> +	  "		[ hkey %x:%x:%x:%x:%x:.... ]\n"
> +	  "		[ hfunc FUNC ]\n"},
>  	{ "-f|--flash", 1, do_flash,
>  	  "Flash firmware image from the specified file to a region on the device",
>  	  "               FILENAME [ REGION-NUMBER-TO-FLASH ]\n" },

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)