Message-ID: <1401673740.14007.141.camel@deadeye.wl.decadent.org.uk>
Date:	Mon, 02 Jun 2014 02:49:00 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Venkat Duvvuru <VenkatKumar.Duvvuru@...lex.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH v5 ethtool 2/2] ethtool: Support for configurable RSS
 hash key

On Mon, 2014-04-21 at 15:38 +0530, Venkat Duvvuru wrote:
[...]
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -874,6 +874,73 @@ static char *unparse_rxfhashopts(u64 opts)
>  	return buf;
>  }
>  
> +static int convert_string_to_hashkey(char *rss_hkey, u32 key_size,
> +				     const char *rss_hkey_string)
> +{
> +	int i = 0;

Type should be u32, consistent with key_size.

> +	int hex_byte;
> +
> +	do {
> +		if (i > (key_size - 1)) {
> +			fprintf(stderr,
> +				"Invalid key: Device supports %d bytes key\n",
> +				key_size);
> +			goto err;
> +		}

Be specific - the key is too long.

> +		if (!(isxdigit(*rss_hkey_string) &&
> +		      isxdigit(*(rss_hkey_string + 1)))) {
> +			fprintf(stderr, "Invalid RSS Hash Key Format\n");
> +			goto err;
> +		}
> +
> +		sscanf(rss_hkey_string, "%2x", &hex_byte);
> +		rss_hkey[i++] = hex_byte;
> +		rss_hkey_string += 2;

I don't like this - validation should be done as part of the conversion,
not separately.  I think this would be better written like:

	int len;
	/* ... */
		if (sscanf(rss_hkey_string, "%2x%n", &hex_byte, &len) < 1 ||
		    len != 2) {
			fprintf(stderr, "Invalid RSS hash key format\n");
			goto err;
		}

(by the way, don't use title-case in error messages).

> +		if (*rss_hkey_string == ':') {
> +			rss_hkey_string++;
> +		} else if (*rss_hkey_string != '\0') {
> +			fprintf(stderr, "Invalid RSS Hash Key Format\n");
> +			goto err;
> +		}

> +	} while (*rss_hkey_string);
> +
> +	if (i != key_size) {
> +		fprintf(stderr, "Invalid key: Device supports %d bytes key\n",
> +			key_size);
> +		goto err;
> +	}

Be specific - the key is too short.

> +	return 0;

This function either returns 0 or exits the program, so why does it have
a return value at all?

> +err:
> +	exit_bad_args();
> +}

I don't think it makes sense to show the general usage error message
after giving a very specific error message.

> static int parse_hkey(char **rss_hkey, u32 key_size,
> 		      const char *rss_hkey_string)
> {
> 	if (!key_size) {
> 		fprintf(stderr,
> 			"Cannot set RX flow hash configuration:\n"
> 			" Hash key setting not supported\n");
> 		exit(1);
> 	}
> 
> 	*rss_hkey = malloc(key_size);
> 	if (!(*rss_hkey))
> 		return -ENOMEM;

Why is that not also fatal?

> 	if (convert_string_to_hashkey(*rss_hkey, key_size,
> 				      rss_hkey_string) < 0) {

Can't happen.

> 		free(*rss_hkey);
> 		*rss_hkey = NULL;
> 		return -EINVAL;
> 	}
> 	return 0;
> }
[...] 
> -static int do_grxfhindir(struct cmd_context *ctx)
> +static int do_grxfh(struct cmd_context *ctx)
>  {
>  	struct ethtool_rxnfc ring_count;
> -	struct ethtool_rxfh_indir indir_head;
> -	struct ethtool_rxfh_indir *indir;
> -	u32 i;
> +	struct ethtool_rxfh rss_head;
> +	struct ethtool_rxfh *rss;
> +	u32 i, indir_bytes;
>  	int err;
> +	char *hkey;
>  
>  	ring_count.cmd = ETHTOOL_GRXRINGS;
>  	err = send_ioctl(ctx, &ring_count);
> @@ -3050,77 +3118,148 @@ static int do_grxfhindir(struct cmd_context *ctx)
>  		return 102;
>  	}
>  
> -	indir_head.cmd = ETHTOOL_GRXFHINDIR;
> -	indir_head.size = 0;
> -	err = send_ioctl(ctx, &indir_head);
> -	if (err < 0) {
> -		perror("Cannot get RX flow hash indirection table size");
> +	rss_head.cmd = ETHTOOL_GRSSH;
> +	rss_head.rss_context = 0;
> +	rss_head.indir_size = 0;
> +	rss_head.key_size = 0;

You need to clear the reserved fields too.  It would be best to
zero-initialise the whole structure and then just set .cmd.

> +	err = send_ioctl(ctx, &rss_head);
> +	if ((err < 0) || (!rss_head.indir_size && !rss_head.key_size)) {
> +		perror("Cannot get RX flow hash indirection and key size");
>  		return 103;
>  	}

You *must* fall back to using ETHTOOL_GRXFHINDIR (and similarly when
setting).  ethtool must continue to work with old kernel versions.
(Without the kernel changes I've just submitted, this also wouldn't work
with any driver other than be2net!)

> -	indir = malloc(sizeof(*indir) +
> -		       indir_head.size * sizeof(*indir->ring_index));
> -	indir->cmd = ETHTOOL_GRXFHINDIR;
> -	indir->size = indir_head.size;
> -	err = send_ioctl(ctx, indir);
> +	rss = calloc(1, sizeof(*rss) +
> +			rss_head.indir_size * sizeof(rss_head.rss_config[0]) +
> +			rss_head.key_size);
> +	rss->cmd = ETHTOOL_GRSSH;
> +	rss->indir_size = rss_head.indir_size;
> +	rss->key_size = rss_head.key_size;
> +	err = send_ioctl(ctx, rss);
>  	if (err < 0) {
> -		perror("Cannot get RX flow hash indirection table");
> +		perror("Cannot get RX flow hash configuration");
>  		return 103;
>  	}
>  
>  	printf("RX flow hash indirection table for %s with %llu RX ring(s):\n",
>  	       ctx->devname, ring_count.data);
> -	for (i = 0; i < indir->size; i++) {
> +	if (!rss->indir_size)
> +		printf("*** Operation Not Supported ***\n");

There is no other place in ethtool we use asterisks for emphasis, and I
don't wish to start.  Please try to be consistent with the existing
style of messages.

[...] 
> -static int do_srxfhindir(struct cmd_context *ctx)
> +
> +
> +

Excess whitespace.

> +static int do_srxfh(struct cmd_context *ctx)
>  {
> +	struct ethtool_rxfh rss_head;
> +	struct ethtool_rxfh *rss;
> +	struct ethtool_rxnfc ring_count;
>  	int rxfhindir_equal = 0;
>  	char **rxfhindir_weight = NULL;
> -	struct ethtool_rxfh_indir indir_head;
> -	struct ethtool_rxfh_indir *indir;
> -	u32 i;
> +	char *rss_hkey = NULL;
>  	int err;
> +	u32 i, arg_num = 0, indir_bytes = 0;
> +	u32 entry_size = sizeof(rss_head.rss_config[0]);
> +	u8 parse_indir = 1;
>  
>  	if (ctx->argc < 2)
>  		exit_bad_args();
> -	if (!strcmp(ctx->argp[0], "equal")) {
> -		if (ctx->argc != 2)
> -			exit_bad_args();
> -		rxfhindir_equal = get_int_range(ctx->argp[1], 0, 1, INT_MAX);
> -	} else if (!strcmp(ctx->argp[0], "weight")) {
> -		rxfhindir_weight = ctx->argp + 1;
> -	} else {
> -		exit_bad_args();
> +
> +	ring_count.cmd = ETHTOOL_GRXRINGS;
> +	err = send_ioctl(ctx, &ring_count);
> +	if (err < 0) {
> +		perror("Cannot get RX ring count");
> +		return 102;
>  	}
>  
> -	indir_head.cmd = ETHTOOL_GRXFHINDIR;
> -	indir_head.size = 0;
> -	err = send_ioctl(ctx, &indir_head);
> +	rss_head.cmd = ETHTOOL_GRSSH;
> +	rss_head.rss_context = 0;
> +	rss_head.indir_size = 0;
> +	rss_head.key_size = 0;
> +	err = send_ioctl(ctx, &rss_head);
>  	if (err < 0) {
> -		perror("Cannot get RX flow hash indirection table size");
> -		return 104;
> +		perror("Cannot get RX flow hash indirection and key size");
> +		return 103;
>  	}
>  
> -	indir = malloc(sizeof(*indir) +
> -		       indir_head.size * sizeof(*indir->ring_index));
> -	indir->cmd = ETHTOOL_SRXFHINDIR;
> -	indir->size = indir_head.size;
> +	if (!strcmp(ctx->argp[0], "hkey")) {
> +		err = parse_hkey(&rss_hkey, rss_head.key_size,
> +				 ctx->argp[1]);
> +		if (err < 0)
> +			return err;
> +
> +		arg_num = 2;
> +		if (!ctx->argp[arg_num])
> +			parse_indir = 0;
> +	}
>  
> +	if (parse_indir) {
> +		if (!strcmp(ctx->argp[arg_num], "equal"))
> +			rxfhindir_equal = get_int_range(ctx->argp[arg_num + 1],
> +							0, 1, INT_MAX);
> +		else if (!strcmp(ctx->argp[arg_num], "weight"))
> +			rxfhindir_weight = ctx->argp + arg_num + 1;
> +		else
> +			exit_bad_args();
> +
> +		indir_bytes = rss_head.indir_size * entry_size;
> +	}
> +
> +	rss = calloc(1, sizeof(*rss) + indir_bytes + rss_head.key_size);
> +	rss->cmd = ETHTOOL_SRSSH;
> +	rss->indir_size = rss_head.indir_size;
> +	rss->key_size = rss_head.key_size;
> +
> +	/*
> +	 * indir_size = 0 ==> reset indir to default
> +	 * indir_size = 0xDEADBEEF ==> ignore indir
> +	 */
>  	if (rxfhindir_equal) {
> -		for (i = 0; i < indir->size; i++)
> -			indir->ring_index[i] = i % rxfhindir_equal;
> -	} else {
> +		for (i = 0; i < rss->indir_size; i++)
> +			rss->rss_config[i] = i % rxfhindir_equal;
> +		arg_num += 2;
> +		if (ctx->argp[arg_num] &&
> +		    !strcmp(ctx->argp[arg_num], "hkey")) {
> +			err = parse_hkey(&rss_hkey, rss_head.key_size,
> +					 ctx->argp[arg_num + 1]);
> +			if (err < 0)
> +				return err;
> +		}
> +	} else if (rxfhindir_weight) {
>  		u32 j, weight, sum = 0, partial = 0;
>  
>  		for (j = 0; rxfhindir_weight[j]; j++) {
> +			if (!strcmp(rxfhindir_weight[j], "hkey")) {
> +				err = parse_hkey(&rss_hkey,
> +						 rss_head.key_size,
> +						 rxfhindir_weight[++j]);
> +				if (err < 0)
> +					return err;
> +				break;
> +			}

The repeated checks for "hkey" is unfortunate.  You should be able to
parse out the keywords using something like this:

	while (arg_num < ctx->argc) {
		if (!strcmp(ctx->argp[arg_num], "equal")) {
			++arg_num;
			rxfhindir_equal = get_int_range(ctx->argp[arg_num],
							0, 1, INT_MAX);
			++arg_num;
		} else if (!strcmp(ctx->argp[arg_num], "weight")) {
			++arg_num;
			rxfhindir_weight = ctx->argp + arg_num;
			while (arg_num < ctx->argc &&
			       isdigit((unsigned char)ctx->argp[arg_num][0]))
				++arg_num;
		} else if (!strcmp(ctx->argp[arg_num], "hkey")) {
			++arg_num;
			rxfhindir_key = ctx->argp + arg_num;
			++arg_num;
		} else {
			exit_bad_args();
		}
	}

and then do the complete validation afterwad.

Also, you obviously didn't run the tests (make check)... you'll need to
make them pass, and add new ones to cover the hkey keyword.

>  			weight = get_u32(rxfhindir_weight[j], 0);
>  			sum += weight;
>  		}
> @@ -3131,7 +3270,7 @@ static int do_srxfhindir(struct cmd_context *ctx)
>  			exit(1);
>  		}
>  
> -		if (sum > indir->size) {
> +		if (sum > rss->indir_size) {
>  			fprintf(stderr,
>  				"Total weight exceeds the size of the "
>  				"indirection table\n");
> @@ -3139,22 +3278,31 @@ static int do_srxfhindir(struct cmd_context *ctx)
>  		}
>  
>  		j = -1;
> -		for (i = 0; i < indir->size; i++) {
> -			while (i >= indir->size * partial / sum) {
> +		for (i = 0; i < rss->indir_size; i++) {
> +			while (i >= rss->indir_size * partial / sum) {
>  				j += 1;
>  				weight = get_u32(rxfhindir_weight[j], 0);
>  				partial += weight;
>  			}
> -			indir->ring_index[i] = j;
> +			rss->rss_config[i] = j;
>  		}
> +	} else {
> +		rss->indir_size = 0xDEADBEEF;

This will need to be changed to ETH_RXFH_INDIR_NO_CHANGE (and the
comment about this magic number needs to be removed).

>  	}
>  
> -	err = send_ioctl(ctx, indir);
> +	if (rss_hkey) {
> +		memcpy((char *)rss->rss_config + indir_bytes,
> +		       rss_hkey, rss->key_size);
> +		free(rss_hkey);
> +	} else {
> +		rss->key_size = 0;
> +	}
> +
> +	err = send_ioctl(ctx, rss);
>  	if (err < 0) {
> -		perror("Cannot set RX flow hash indirection table");
> +		perror("Cannot set RX flow hash configuration");
>  		return 105;
>  	}
> -
>  	return 0;
>  }
>  
> @@ -3833,11 +3981,12 @@ static const struct option {
>  	  "		delete %d\n" },
>  	{ "-T|--show-time-stamping", 1, do_tsinfo,
>  	  "Show time stamping capabilities" },
> -	{ "-x|--show-rxfh-indir", 1, do_grxfhindir,
> -	  "Show Rx flow hash indirection" },
> -	{ "-X|--set-rxfh-indir", 1, do_srxfhindir,
> -	  "Set Rx flow hash indirection",
> -	  "		equal N | weight W0 W1 ...\n" },
> +	{ "-x|--show-rxfh-indir|--show-rxfh", 1, do_grxfh,
> +	  "Show Rx flow hash indirection and/or hashkey" },
> +	{ "-X|--set-rxfh-indir|--rxfh", 1, do_srxfh,
> +	  "Set Rx flow hash indirection and/or hashkey",
> +	  "		equal N | weight W0 W1 ...\n"

The indirection table part is also optional now, so you should put
brackets around that.

> +	  "		[ hkey %x:%x:%x:%x:%x:.... ]\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.

-- 
Ben Hutchings
Any smoothly functioning technology is indistinguishable from a rigged demo.

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

Powered by blists - more mailing lists