netdev - RE: [PATCH v2 3/6] ethtool: introduce new ioctl for per-queue settings

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: <146CDE3B8C383A4B88452B498CB0FBBBFD9C3F87@ORSMSX157.amr.corp.intel.com>
Date:   Thu, 7 Feb 2019 23:37:19 +0000
From:   "Nunley, Nicholas D" <nicholas.d.nunley@...el.com>
To:     Michal Kubecek <mkubecek@...e.cz>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "linville@...driver.com" <linville@...driver.com>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>
Subject: RE: [PATCH v2 3/6] ethtool: introduce new ioctl for per-queue
 settings


> -----Original Message-----
> From: Michal Kubecek [mailto:mkubecek@...e.cz]
> Sent: Wednesday, February 6, 2019 1:19 AM
> To: netdev@...r.kernel.org
> Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>; linville@...driver.com;
> Nunley, Nicholas D <nicholas.d.nunley@...el.com>; nhorman@...hat.com;
> sassmann@...hat.com
> Subject: Re: [PATCH v2 3/6] ethtool: introduce new ioctl for per-queue
> settings
> 
> On Tue, Feb 05, 2019 at 04:01:03PM -0800, Jeff Kirsher wrote:
> > Introduce a new ioctl for setting per-queue parameters.
> > Users can apply commands to specific queues by setting SUB_COMMAND
> and
> > queue_mask with the following ethtool command:
> >
> >  ethtool --set-perqueue-command DEVNAME [queue_mask %x]
> SUB_COMMAND
> 
> The "set" part is IMHO a bit confusing in combination with "show" type
> subcommands.

I'm not a fan of the "set" either. This patch set had already gone through some review before it was passed on to me, and the command naming wasn't a previous point of contention and I didn't want to disturb the parts that weren't "broken", but since you've brought it up I agree that this may not be the best name. I think "--perqueue-command" is more appropriate. Does this seem reasonable to you?

> > +static int set_queue_mask(u32 *queue_mask, char *str) {
> > +	int len = strlen(str);
> > +	int index = __KERNEL_DIV_ROUND_UP(len * 4, 32);
> > +	char tmp[9];
> > +	char *end = str + len;
> > +	int i, num;
> > +	__u32 mask;
> > +	int n_queues = 0;
> > +
> > +	if (len > MAX_NUM_QUEUE)
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < index; i++) {
> > +		num = end - str;
> > +
> > +		if (num >= 8) {
> > +			end -= 8;
> > +			num = 8;
> > +		} else {
> > +			end = str;
> > +		}
> > +		strncpy(tmp, end, num);
> > +		tmp[num] = '\0';
> > +
> > +		queue_mask[i] = strtoul(tmp, NULL, 16);
> > +
> > +		mask = queue_mask[i];
> > +		while (mask > 0) {
> > +			if (mask & 0x1)
> > +				n_queues++;
> > +			mask = mask >> 1;
> > +		}
> > +	}
> > +
> > +	return n_queues;
> > +}
> 
> Could you allow optional prefix "0x" as we do for link modes? Maybe
> parse_hex_u32_bitmap() could be used for both.

It's supposed to already work like this through to the eventual call to strtoul() (any "0x" prefix is ignored), but after closer inspection the current implementation won't work for a very specific set of inputs. Depending on the alignment the bitmap substring passed into strtoul() can end up being unrecognizable. For instance, "0xFFFFFFF" ends up getting divided into two calls to strtoul(), "xFFFFFFF" and "0", the former of which ends up evaluating to 0x0 rather than 0xFFFFFFF. Per your suggestion I'll replace the set_queue_mask() functionality with a call to parse_hex_u32_bitmap() to generate the bitmap and avoid this mess, plus some additional code to count up the number of queues.

> 
> > +static int do_perqueue(struct cmd_context *ctx) {
> > +	__u32
> queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)] = {0};
> > +	int i, n_queues = 0;
> > +
> > +	if (ctx->argc == 0)
> > +		exit_bad_args();
> > +
> > +	/*
> > +	 * The sub commands will be applied to
> > +	 * all queues if no queue_mask set
> > +	 */
> > +	if (strncmp(*ctx->argp, "queue_mask", 10)) {
> > +		n_queues = find_max_num_queues(ctx);
> > +		if (n_queues < 0) {
> > +			perror("Cannot get number of queues");
> > +			return -EFAULT;
> > +		}
> > +		for (i = 0; i < n_queues / 32; i++)
> > +			queue_mask[i] = ~0;
> > +		queue_mask[i] = (1 << (n_queues - i * 32)) - 1;
> > +		fprintf(stdout,
> > +			"The sub commands will be applied to all %d
> queues\n",
> > +			n_queues);
> > +	} else {
> > +		ctx->argc--;
> > +		ctx->argp++;
> > +		n_queues = set_queue_mask(queue_mask, *ctx->argp);
> > +		if (n_queues < 0) {
> > +			perror("Invalid queue mask");
> > +			return n_queues;
> > +		}
> > +		ctx->argc--;
> > +		ctx->argp++;
> > +	}
> > +
> > +	i = find_option(ctx->argc, ctx->argp);
> > +	if (i < 0)
> > +		exit_bad_args();
> > +
> > +	/* no sub_command support yet */
> > +
> > +	return 0;
> > +}
> 
> Technically the return value should be -EOPNOTSUPP here but as the next
> patch fixes that, it doesn't really matter.

I'll fix this anyway since I'll be submitting a new revision.

Thanks.