| Message ID | 20190206000106.24364-3-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | John Linville |
| Headers | show |
| Series | [v2,1/6] ethtool: move option parsing related code into function | expand |
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
On Tue, Feb 05, 2019 at 04:01:03PM -0800, Jeff Kirsher wrote: > +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(MAX(echannels.rx_count, echannels.tx_count), > + echannels.combined_count); > +} Is the outer MAX() correct here? From the documentation to -L option, it rather seems we might want return MAX(echannels.rx_count, echannels.tx_count) + echannels.combined_count; But I can't find any NIC around which would have non-zero rx_count or tx_count so that I cannot check. Michal Kubecek
On Tue, Feb 05, 2019 at 04:01:03PM -0800, Jeff Kirsher wrote: > +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)) { This would match any string starting with "queue_mask", is it intended? > + 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; It's unlikely today, I guess, but in theory, this would overflow if n_queues == MAX_NUM_QUEUE > + 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; > +} Michal Kubecek
> -----Original Message----- > From: Michal Kubecek [mailto:mkubecek@suse.cz] > Sent: Wednesday, February 6, 2019 1:19 AM > To: netdev@vger.kernel.org > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; linville@tuxdriver.com; > Nunley, Nicholas D <nicholas.d.nunley@intel.com>; nhorman@redhat.com; > sassmann@redhat.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.
> -----Original Message----- > From: Michal Kubecek [mailto:mkubecek@suse.cz] > Sent: Wednesday, February 6, 2019 2:33 AM > To: netdev@vger.kernel.org > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; linville@tuxdriver.com; > Nunley, Nicholas D <nicholas.d.nunley@intel.com>; nhorman@redhat.com; > sassmann@redhat.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: > > +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(MAX(echannels.rx_count, echannels.tx_count), > > + echannels.combined_count); > > +} > > Is the outer MAX() correct here? From the documentation to -L option, it > rather seems we might want > > return MAX(echannels.rx_count, echannels.tx_count) + > echannels.combined_count; > > But I can't find any NIC around which would have non-zero rx_count or > tx_count so that I cannot check. I think the original assumption here must have been that drivers either use separate Tx/Rx channels or support the combined approach, but never both at the same time. All Intel drivers only support the combined method so I didn't think to second guess this detail of the original implementation, however, I've since looked through the uses of get/set_channels elsewhere in the kernel and it looks like there are a few drivers that support both methods simultaneously, so that clearly needs to be supported too. Your suggestion above looks like the right thing to do. The device queue-index-to-channel mapping could be a little ambiguous in the mixed mode (and is left as an implementation detail of the individual driver), but I suppose the most sensible approach would be to index through the combined channels first, then move on to the individual Tx/Rx channels, so there shouldn't be any future issues here if these drivers want to add support for per-queue commands.
> -----Original Message----- > From: Michal Kubecek [mailto:mkubecek@suse.cz] > Sent: Wednesday, February 6, 2019 4:43 AM > To: netdev@vger.kernel.org > Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; linville@tuxdriver.com; > Nunley, Nicholas D <nicholas.d.nunley@intel.com>; nhorman@redhat.com; > sassmann@redhat.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: > > +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)) { > > This would match any string starting with "queue_mask", is it intended? No, I'll fix this. I don't know that there are any use cases where this distinction would matter, but then again there's no reason to have it match this way. > > > + 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; > > It's unlikely today, I guess, but in theory, this would overflow if n_queues == > MAX_NUM_QUEUE Nice catch, I'll add a check to avoid this. > > + 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; > > +} > > Michal Kubecek
On Thu, Feb 07, 2019 at 11:37:19PM +0000, Nunley, Nicholas D wrote: > From: Michal Kubecek [mailto:mkubecek@suse.cz] > > 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? That sounds good, I would say either that or "--per-queue". Michal Kubecek
On Fri, Feb 8, 2019 at 2:04 AM Michal Kubecek <mkubecek@suse.cz> wrote: > > On Thu, Feb 07, 2019 at 11:37:19PM +0000, Nunley, Nicholas D wrote: > > From: Michal Kubecek [mailto:mkubecek@suse.cz] > > > 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? > > That sounds good, I would say either that or "--per-queue". +1 --per-queue is short and easy to remember. And perhaps reserve short-hand '-Q'? I expect this to become a popular option. Especially if perhaps eventually it can dump per queue stats (-S).
diff --git a/ethtool.8.in b/ethtool.8.in index 5a26cff..0aaca2c 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 \-\-set\-perqueue\-command +.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 \-\-set\-perqueue\-command +Sets 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 +Sets the sub command. +.RE .SH BUGS Not supported (in part or whole) on all network drivers. .SH AUTHOR diff --git a/ethtool.c b/ethtool.c index af266c5..4dc725c 100644 --- a/ethtool.c +++ b/ethtool.c @@ -5048,6 +5048,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) { @@ -5243,6 +5245,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"}, + { "--set-perqueue-command", 1, do_perqueue, "Set per queue command", + " [queue_mask %x] SUB_COMMAND\n"}, { "-h|--help", 0, show_usage, "Show this help" }, { "--version", 0, do_version, "Show version number" }, {} @@ -5294,6 +5298,103 @@ static int find_option(int argc, char **argp) return -1; } +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; +} + +#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(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", 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; +} + int main(int argc, char **argp) { int (*func)(struct cmd_context *);