| Message ID | 20190206000106.24364-4-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:04PM -0800, Jeff Kirsher wrote: > diff --git a/ethtool.c b/ethtool.c > index 4dc725c..9a1b83b 100644 > --- a/ethtool.c > +++ b/ethtool.c > @@ -1409,6 +1409,29 @@ static int dump_coalesce(const struct ethtool_coalesce *ecoal) > return 0; > } > > +void dump_per_queue_coalesce(struct ethtool_per_queue_op *per_queue_opt, > + __u32 *queue_mask) > +{ > + char *addr; > + int i; > + > + addr = (char *)per_queue_opt + sizeof(*per_queue_opt); > + for (i = 0; i < __KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32); i++) { > + int queue = i * 32; > + __u32 mask = queue_mask[i]; > + > + while (mask > 0) { > + if (mask & 0x1) { > + fprintf(stdout, "Queue: %d\n", queue); > + dump_coalesce((struct ethtool_coalesce *)addr); > + addr += sizeof(struct ethtool_coalesce); > + } > + mask = mask >> 1; > + queue++; > + } > + } > +} > + > struct feature_state { > u32 off_flags; > struct ethtool_gfeatures features; The casts and pointer arithmetic are a bit complicated. How about this? struct ethtool_coalesce *addr; ... addr = (struct ethtool_coalesce *)(per_queue_opt + 1); ... dump_coalesce(addr); addr++; Also passing n_queue down from do_perqueue() would prevent having to parse the whole bitmap even if we already know the NIC has only few queues. > @@ -5245,7 +5268,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", > + { "--set-perqueue-command", 1, do_perqueue, "Set per queue command. " > + "The supported sub commands include --show-coalesce", > " [queue_mask %x] SUB_COMMAND\n"}, > { "-h|--help", 0, show_usage, "Show this help" }, > { "--version", 0, do_version, "Show version number" }, > @@ -5350,8 +5374,32 @@ static int find_max_num_queues(struct cmd_context *ctx) > echannels.combined_count); > } > > +static struct ethtool_per_queue_op * > +get_per_queue_coalesce(struct cmd_context *ctx, __u32 *queue_mask, int n_queues) > +{ > + struct ethtool_per_queue_op *per_queue_opt; > + > + per_queue_opt = malloc(sizeof(*per_queue_opt) + n_queues * > + sizeof(struct ethtool_coalesce)); > + if (!per_queue_opt) > + return NULL; > + > + memcpy(per_queue_opt->queue_mask, queue_mask, > + __KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32) * sizeof(__u32)); > + per_queue_opt->cmd = ETHTOOL_PERQUEUE; > + per_queue_opt->sub_command = ETHTOOL_GCOALESCE; > + if (send_ioctl(ctx, per_queue_opt)) { > + free(per_queue_opt); > + perror("Cannot get device per queue parameters"); > + return NULL; > + } > + > + return per_queue_opt; > +} > + > static int do_perqueue(struct cmd_context *ctx) > { > + struct ethtool_per_queue_op *per_queue_opt; > __u32 queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)] = {0}; > int i, n_queues = 0; > > @@ -5390,7 +5438,19 @@ static int do_perqueue(struct cmd_context *ctx) > if (i < 0) > exit_bad_args(); > > - /* no sub_command support yet */ > + if (strstr(args[i].opts, "--show-coalesce") != NULL) { Comparing args[i].func to do_gcoalesce might be easier. > + per_queue_opt = get_per_queue_coalesce(ctx, queue_mask, > + n_queues); > + if (per_queue_opt == NULL) { > + perror("Cannot get device per queue parameters"); > + return -EFAULT; > + } > + dump_per_queue_coalesce(per_queue_opt, queue_mask); > + free(per_queue_opt); > + } else { > + perror("The subcommand is not supported yet"); > + return -EOPNOTSUPP; > + } > > return 0; > } Michal Kubecek
> -----Original Message----- > From: Michal Kubecek [mailto:mkubecek@suse.cz] > Sent: Wednesday, February 6, 2019 5:22 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 4/6] ethtool: support per-queue sub command -- > show-coalesce > > On Tue, Feb 05, 2019 at 04:01:04PM -0800, Jeff Kirsher wrote: > > diff --git a/ethtool.c b/ethtool.c > > index 4dc725c..9a1b83b 100644 > > --- a/ethtool.c > > +++ b/ethtool.c > > @@ -1409,6 +1409,29 @@ static int dump_coalesce(const struct > ethtool_coalesce *ecoal) > > return 0; > > } > > > > +void dump_per_queue_coalesce(struct ethtool_per_queue_op > *per_queue_opt, > > + __u32 *queue_mask) > > +{ > > + char *addr; > > + int i; > > + > > + addr = (char *)per_queue_opt + sizeof(*per_queue_opt); > > + for (i = 0; i < __KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32); > i++) { > > + int queue = i * 32; > > + __u32 mask = queue_mask[i]; > > + > > + while (mask > 0) { > > + if (mask & 0x1) { > > + fprintf(stdout, "Queue: %d\n", queue); > > + dump_coalesce((struct ethtool_coalesce > *)addr); > > + addr += sizeof(struct ethtool_coalesce); > > + } > > + mask = mask >> 1; > > + queue++; > > + } > > + } > > +} > > + > > struct feature_state { > > u32 off_flags; > > struct ethtool_gfeatures features; > > The casts and pointer arithmetic are a bit complicated. How about this? > > struct ethtool_coalesce *addr; > ... > addr = (struct ethtool_coalesce *)(per_queue_opt + 1); ... > dump_coalesce(addr); > addr++; > > Also passing n_queue down from do_perqueue() would prevent having to > parse the whole bitmap even if we already know the NIC has only few > queues. Okay, that does make the pointer arithmetic a little more straightforward, so I'll change this. I'll also add the n_queue parameter to set_per_queue_coalesce() and dump_per_queue_coalesce() so we can exit early once we've found all the queues. > > @@ -5245,7 +5268,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", > > + { "--set-perqueue-command", 1, do_perqueue, "Set per queue > command. " > > + "The supported sub commands include --show-coalesce", > > " [queue_mask %x] SUB_COMMAND\n"}, > > { "-h|--help", 0, show_usage, "Show this help" }, > > { "--version", 0, do_version, "Show version number" }, @@ -5350,8 > > +5374,32 @@ static int find_max_num_queues(struct cmd_context *ctx) > > echannels.combined_count); > > } > > > > +static struct ethtool_per_queue_op * > > +get_per_queue_coalesce(struct cmd_context *ctx, __u32 *queue_mask, > > +int n_queues) { > > + struct ethtool_per_queue_op *per_queue_opt; > > + > > + per_queue_opt = malloc(sizeof(*per_queue_opt) + n_queues * > > + sizeof(struct ethtool_coalesce)); > > + if (!per_queue_opt) > > + return NULL; > > + > > + memcpy(per_queue_opt->queue_mask, queue_mask, > > + __KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32) * > sizeof(__u32)); > > + per_queue_opt->cmd = ETHTOOL_PERQUEUE; > > + per_queue_opt->sub_command = ETHTOOL_GCOALESCE; > > + if (send_ioctl(ctx, per_queue_opt)) { > > + free(per_queue_opt); > > + perror("Cannot get device per queue parameters"); > > + return NULL; > > + } > > + > > + return per_queue_opt; > > +} > > + > > static int do_perqueue(struct cmd_context *ctx) { > > + struct ethtool_per_queue_op *per_queue_opt; > > __u32 > queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)] = {0}; > > int i, n_queues = 0; > > > > @@ -5390,7 +5438,19 @@ static int do_perqueue(struct cmd_context *ctx) > > if (i < 0) > > exit_bad_args(); > > > > - /* no sub_command support yet */ > > + if (strstr(args[i].opts, "--show-coalesce") != NULL) { > > Comparing args[i].func to do_gcoalesce might be easier. This is the one comment where I think it's better to leave the code as it is. To me is seems more confusing to match on a function pointer that we're never going to call. Unless there are more objections I'd rather keep it the way it is. Otherwise, thanks for the review and all the comments. I'll work on making these changes and get an updated version submitted. Nick > > > + per_queue_opt = get_per_queue_coalesce(ctx, > queue_mask, > > + n_queues); > > + if (per_queue_opt == NULL) { > > + perror("Cannot get device per queue parameters"); > > + return -EFAULT; > > + } > > + dump_per_queue_coalesce(per_queue_opt, queue_mask); > > + free(per_queue_opt); > > + } else { > > + perror("The subcommand is not supported yet"); > > + return -EOPNOTSUPP; > > + } > > > > return 0; > > } > > Michal Kubecek
On Thu, Feb 07, 2019 at 11:53:40PM +0000, Nunley, Nicholas D wrote: > > > @@ -5390,7 +5438,19 @@ static int do_perqueue(struct cmd_context *ctx) > > > if (i < 0) > > > exit_bad_args(); > > > > > > - /* no sub_command support yet */ > > > + if (strstr(args[i].opts, "--show-coalesce") != NULL) { > > > > Comparing args[i].func to do_gcoalesce might be easier. > > This is the one comment where I think it's better to leave the code as it is. > To me is seems more confusing to match on a function pointer that we're never > going to call. Unless there are more objections I'd rather keep it the way it > is. No problem. This is not a code where performance is crucial. In theory, you could get into trouble if someone introduces another command (allowing per queue settings) with name like "--show-coalesce-foo" but that's not very likely, IMHO. Michal Kubecek
diff --git a/ethtool.8.in b/ethtool.8.in index 0aaca2c..71bb962 100644 --- a/ethtool.8.in +++ b/ethtool.8.in @@ -1153,7 +1153,7 @@ 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. +Sets the sub command. The supported sub commands include --show-coalesce. .RE .SH BUGS Not supported (in part or whole) on all network drivers. diff --git a/ethtool.c b/ethtool.c index 4dc725c..9a1b83b 100644 --- a/ethtool.c +++ b/ethtool.c @@ -1409,6 +1409,29 @@ static int dump_coalesce(const struct ethtool_coalesce *ecoal) return 0; } +void dump_per_queue_coalesce(struct ethtool_per_queue_op *per_queue_opt, + __u32 *queue_mask) +{ + char *addr; + int i; + + addr = (char *)per_queue_opt + sizeof(*per_queue_opt); + for (i = 0; i < __KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32); i++) { + int queue = i * 32; + __u32 mask = queue_mask[i]; + + while (mask > 0) { + if (mask & 0x1) { + fprintf(stdout, "Queue: %d\n", queue); + dump_coalesce((struct ethtool_coalesce *)addr); + addr += sizeof(struct ethtool_coalesce); + } + mask = mask >> 1; + queue++; + } + } +} + struct feature_state { u32 off_flags; struct ethtool_gfeatures features; @@ -5245,7 +5268,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", + { "--set-perqueue-command", 1, do_perqueue, "Set per queue command. " + "The supported sub commands include --show-coalesce", " [queue_mask %x] SUB_COMMAND\n"}, { "-h|--help", 0, show_usage, "Show this help" }, { "--version", 0, do_version, "Show version number" }, @@ -5350,8 +5374,32 @@ static int find_max_num_queues(struct cmd_context *ctx) echannels.combined_count); } +static struct ethtool_per_queue_op * +get_per_queue_coalesce(struct cmd_context *ctx, __u32 *queue_mask, int n_queues) +{ + struct ethtool_per_queue_op *per_queue_opt; + + per_queue_opt = malloc(sizeof(*per_queue_opt) + n_queues * + sizeof(struct ethtool_coalesce)); + if (!per_queue_opt) + return NULL; + + memcpy(per_queue_opt->queue_mask, queue_mask, + __KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32) * sizeof(__u32)); + per_queue_opt->cmd = ETHTOOL_PERQUEUE; + per_queue_opt->sub_command = ETHTOOL_GCOALESCE; + if (send_ioctl(ctx, per_queue_opt)) { + free(per_queue_opt); + perror("Cannot get device per queue parameters"); + return NULL; + } + + return per_queue_opt; +} + static int do_perqueue(struct cmd_context *ctx) { + struct ethtool_per_queue_op *per_queue_opt; __u32 queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)] = {0}; int i, n_queues = 0; @@ -5390,7 +5438,19 @@ static int do_perqueue(struct cmd_context *ctx) if (i < 0) exit_bad_args(); - /* no sub_command support yet */ + if (strstr(args[i].opts, "--show-coalesce") != NULL) { + per_queue_opt = get_per_queue_coalesce(ctx, queue_mask, + n_queues); + if (per_queue_opt == NULL) { + perror("Cannot get device per queue parameters"); + return -EFAULT; + } + dump_per_queue_coalesce(per_queue_opt, queue_mask); + free(per_queue_opt); + } else { + perror("The subcommand is not supported yet"); + return -EOPNOTSUPP; + } return 0; }