netdev - RE: [PATCH v3 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]

Date:   Fri, 1 Mar 2019 23:35:47 +0000
From:   "Nunley, Nicholas D" <nicholas.d.nunley@...el.com>
To:     Michal Kubecek <mkubecek@...e.cz>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:     "linville@...driver.com" <linville@...driver.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH v3 3/6] ethtool: introduce new ioctl for per-queue
 settings

> -----Original Message-----
> From: Michal Kubecek [mailto:mkubecek@...e.cz]
> Sent: Friday, March 1, 2019 6:18 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>
> Cc: linville@...driver.com; Nunley, Nicholas D
> <nicholas.d.nunley@...el.com>; netdev@...r.kernel.org
> Subject: Re: [PATCH v3 3/6] ethtool: introduce new ioctl for per-queue
> settings
> 
> On Fri, Mar 01, 2019 at 12:15:29AM -0800, Jeff Kirsher wrote:
> > From: Nicholas Nunley <nicholas.d.nunley@...el.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@...el.com>
> >
> > Signed-off-by: Nicholas Nunley <nicholas.d.nunley@...el.com>
> > ---
> 
> Reviewed-by: Michal Kubecek <mkubecek@...e.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).

I'll add the parentheses since I'm sure others would prefer it that way as well, even if it's unlikely to make a functional difference in this case.

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

Okay, I'll print a message that makes this limit clearer to the user.

I'll also fix the missing space you pointed out in the other patch and resubmit the series.

Thanks for the review.

Nick