| Message ID | 20190301081532.11771-3-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | John Linville |
| Headers | show |
| Series | [v3,1/6] ethtool: move option parsing related code into function | expand |
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
> -----Original Message----- > From: Michal Kubecek [mailto:mkubecek@suse.cz] > Sent: Friday, March 1, 2019 6:18 AM > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com> > Cc: linville@tuxdriver.com; Nunley, Nicholas D > <nicholas.d.nunley@intel.com>; netdev@vger.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@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). 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
diff --git a/ethtool.8.in b/ethtool.8.in index 5a26cff..10f24db 100644 --- a/ethtool.8.in +++ b/ethtool.8.in @@ -391,6 +391,14 @@ ethtool \- query or control network driver and hardware settings .I devname .B encoding .BR auto | off | rs | baser \ [...] +.HP +.B ethtool \-Q|\-\-per\-queue +.I devname +.RB [ queue_mask +.IR %x ] +.I sub_command +.RB ... + . . .\" Adjust lines (i.e. full justification) and hyphenate. .ad @@ -1135,6 +1143,18 @@ RS Force RS-FEC encoding BaseR Force BaseR encoding .TE .RE +.TP +.B \-Q|\-\-per\-queue +Applies provided sub command to specific queues. +.RS 4 +.TP +.B queue_mask %x +Sets the specific queues which the sub command is applied to. +If queue_mask is not set, the sub command will be applied to all queues. +.TP +.B sub_command +Sub command to apply. +.RE .SH BUGS Not supported (in part or whole) on all network drivers. .SH AUTHOR 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) + +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; + } + for (i = 0; i < n_queues / 32; i++) + queue_mask[i] = ~0; + if (n_queues % 32) + 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++; + if (parse_hex_u32_bitmap(*ctx->argp, MAX_NUM_QUEUE, + queue_mask)) { + fprintf(stdout, "Invalid queue mask\n"); + return -1; + } + for (i = 0; i < __KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32); i++) { + __u32 mask = queue_mask[i]; + + while (mask > 0) { + if (mask & 0x1) + n_queues++; + mask = mask >> 1; + } + } + ctx->argc--; + ctx->argp++; + } + + i = find_option(ctx->argc, ctx->argp); + if (i < 0) + exit_bad_args(); + + /* no sub_command support yet */ + + return -EOPNOTSUPP; +} + int main(int argc, char **argp) { int (*func)(struct cmd_context *);