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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190206091832.GI21401@unicorn.suse.cz>
Date:   Wed, 6 Feb 2019 10:18:32 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>, linville@...driver.com,
        Nicholas Nunley <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.

> +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.

> +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.

Michal Kubecek