Re: [PATCH v3 3/6] ethtool: introduce new ioctl for per-queue settings

[Michal Kubecek <mkubecek@suse.cz>]

On Fri, Mar 01, 2019 at 12:15:29AM -0800, Jeff Kirsher wrote:
> From: Nicholas Nunley <nicholas.d.nunley@intel.com>
> 
> Introduce a new ioctl for applying per-queue parameters.
> Users can apply commands to specific queues by setting SUB_COMMAND and
> queue_mask with the following ethtool command:
> 
>  ethtool -Q|--per-queue DEVNAME [queue_mask %x] SUB_COMMAND
> 
> If queue_mask is not set, the SUB_COMMAND will be applied to all queues.
> 
> SUB_COMMANDs for per-queue settings will be implemented in following
> patches.
> 
> Based on patch by Kan Liang <kan.liang@intel.com>
> 
> Signed-off-by: Nicholas Nunley <nicholas.d.nunley@intel.com>
> ---

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>

> diff --git a/ethtool.c b/ethtool.c
> index d9850f4..5f72741 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -5051,6 +5051,8 @@ static int do_sfec(struct cmd_context *ctx)
>  	return 0;
>  }
>  
> +static int do_perqueue(struct cmd_context *ctx);
> +
>  #ifndef TEST_ETHTOOL
>  int send_ioctl(struct cmd_context *ctx, void *cmd)
>  {
> @@ -5246,6 +5248,8 @@ static const struct option {
>  	{ "--show-fec", 1, do_gfec, "Show FEC settings"},
>  	{ "--set-fec", 1, do_sfec, "Set FEC settings",
>  	  "		[ encoding auto|off|rs|baser [...]]\n"},
> +	{ "-Q|--per-queue", 1, do_perqueue, "Apply per-queue command",
> +	  "             [queue_mask %x] SUB_COMMAND\n"},
>  	{ "-h|--help", 0, show_usage, "Show this help" },
>  	{ "--version", 0, do_version, "Show version number" },
>  	{}
> @@ -5297,6 +5301,77 @@ static int find_option(int argc, char **argp)
>  	return -1;
>  }
>  
> +#define MAX(x, y) (x > y ? x : y)

The unparenthesised macro arguments give me goosebumps but I couldn't
come with an example where it would be wrong (without using assignment
operators in the arguments which would be bad idea on its own).

> +
> +static int find_max_num_queues(struct cmd_context *ctx)
> +{
> +	struct ethtool_channels echannels;
> +
> +	echannels.cmd = ETHTOOL_GCHANNELS;
> +	if (send_ioctl(ctx, &echannels))
> +		return -1;
> +
> +	return MAX(echannels.rx_count, echannels.tx_count) +
> +		echannels.combined_count;
> +}
> +
> +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", 11)) {
> +		n_queues = find_max_num_queues(ctx);
> +		if (n_queues < 0) {
> +			perror("Cannot get number of queues");
> +			return -EFAULT;
> +		} else if (n_queues > MAX_NUM_QUEUE) {
> +			n_queues = MAX_NUM_QUEUE;
> +		}

Perhaps a warning should issued in such case (theoretical right now and
in near future) to tell user the settings are only a subset of queues
will be used.

Michal Kubecek