[PATCH v3 1/6] ethtool: move option parsing related code into function

[PATCH v3 1/6] ethtool: move option parsing related code into function

[Jeff Kirsher]

From: Nicholas Nunley <nicholas.d.nunley@intel.com>

Move option parsing code into find_option function.

No behavior changes.

Based on patch by Kan Liang <kan.liang@intel.com>

Signed-off-by: Nicholas Nunley <nicholas.d.nunley@intel.com>
---
 ethtool.c | 49 +++++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index fb4c088..917626f 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5268,6 +5268,29 @@ static int show_usage(struct cmd_context *ctx)
 	return 0;
 }
 
+static int find_option(int argc, char **argp)
+{
+	const char *opt;
+	size_t len;
+	int k;
+
+	for (k = 0; args[k].opts; k++) {
+		opt = args[k].opts;
+		for (;;) {
+			len = strcspn(opt, "|");
+			if (strncmp(*argp, opt, len) == 0 &&
+			    (*argp)[len] == 0)
+				return k;
+
+			if (opt[len] == 0)
+				break;
+			opt += len + 1;
+		}
+	}
+
+	return -1;
+}
+
 int main(int argc, char **argp)
 {
 	int (*func)(struct cmd_context *);
@@ -5287,24 +5310,14 @@ int main(int argc, char **argp)
 	 */
 	if (argc == 0)
 		exit_bad_args();
-	for (k = 0; args[k].opts; k++) {
-		const char *opt;
-		size_t len;
-		opt = args[k].opts;
-		for (;;) {
-			len = strcspn(opt, "|");
-			if (strncmp(*argp, opt, len) == 0 &&
-			    (*argp)[len] == 0) {
-				argp++;
-				argc--;
-				func = args[k].func;
-				want_device = args[k].want_device;
-				goto opt_found;
-			}
-			if (opt[len] == 0)
-				break;
-			opt += len + 1;
-		}
+
+	k = find_option(argc, argp);
+	if (k >= 0) {
+		argp++;
+		argc--;
+		func = args[k].func;
+		want_device = args[k].want_device;
+		goto opt_found;
 	}
 	if ((*argp)[0] == '-')
 		exit_bad_args();
-- 
2.20.1

[PATCH v3 2/6] ethtool: move cmdline_coalesce out of do_scoalesce

[Jeff Kirsher]

From: Nicholas Nunley <nicholas.d.nunley@intel.com>

Move the definition of cmdline_coalesce out of do_scoalesce and into a
macro so it can be resused across functions.

No behavior change.

Based on patch by Kan Liang <kan.liang@intel.com>

Signed-off-by: Nicholas Nunley <nicholas.d.nunley@intel.com>
---
 ethtool.c | 142 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 74 insertions(+), 68 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 917626f..d9850f4 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2101,78 +2101,84 @@ static int do_gcoalesce(struct cmd_context *ctx)
 	return 0;
 }
 
+#define DECLARE_COALESCE_OPTION_VARS()		\
+	s32 coal_stats_wanted = -1;		\
+	int coal_adaptive_rx_wanted = -1;	\
+	int coal_adaptive_tx_wanted = -1;	\
+	s32 coal_sample_rate_wanted = -1;	\
+	s32 coal_pkt_rate_low_wanted = -1;	\
+	s32 coal_pkt_rate_high_wanted = -1;	\
+	s32 coal_rx_usec_wanted = -1;		\
+	s32 coal_rx_frames_wanted = -1;		\
+	s32 coal_rx_usec_irq_wanted = -1;	\
+	s32 coal_rx_frames_irq_wanted = -1;	\
+	s32 coal_tx_usec_wanted = -1;		\
+	s32 coal_tx_frames_wanted = -1;		\
+	s32 coal_tx_usec_irq_wanted = -1;	\
+	s32 coal_tx_frames_irq_wanted = -1;	\
+	s32 coal_rx_usec_low_wanted = -1;	\
+	s32 coal_rx_frames_low_wanted = -1;	\
+	s32 coal_tx_usec_low_wanted = -1;	\
+	s32 coal_tx_frames_low_wanted = -1;	\
+	s32 coal_rx_usec_high_wanted = -1;	\
+	s32 coal_rx_frames_high_wanted = -1;	\
+	s32 coal_tx_usec_high_wanted = -1;	\
+	s32 coal_tx_frames_high_wanted = -1
+
+#define COALESCE_CMDLINE_INFO(__ecoal)					\
+{									\
+	{ "adaptive-rx", CMDL_BOOL, &coal_adaptive_rx_wanted,		\
+	  &__ecoal.use_adaptive_rx_coalesce },				\
+	{ "adaptive-tx", CMDL_BOOL, &coal_adaptive_tx_wanted,		\
+	  &__ecoal.use_adaptive_tx_coalesce },				\
+	{ "sample-interval", CMDL_S32, &coal_sample_rate_wanted,	\
+	  &__ecoal.rate_sample_interval },				\
+	{ "stats-block-usecs", CMDL_S32, &coal_stats_wanted,		\
+	  &__ecoal.stats_block_coalesce_usecs },			\
+	{ "pkt-rate-low", CMDL_S32, &coal_pkt_rate_low_wanted,		\
+	  &__ecoal.pkt_rate_low },					\
+	{ "pkt-rate-high", CMDL_S32, &coal_pkt_rate_high_wanted,	\
+	  &__ecoal.pkt_rate_high },					\
+	{ "rx-usecs", CMDL_S32, &coal_rx_usec_wanted,			\
+	  &__ecoal.rx_coalesce_usecs },					\
+	{ "rx-frames", CMDL_S32, &coal_rx_frames_wanted,		\
+	  &__ecoal.rx_max_coalesced_frames },				\
+	{ "rx-usecs-irq", CMDL_S32, &coal_rx_usec_irq_wanted,		\
+	  &__ecoal.rx_coalesce_usecs_irq },				\
+	{ "rx-frames-irq", CMDL_S32, &coal_rx_frames_irq_wanted,	\
+	  &__ecoal.rx_max_coalesced_frames_irq },			\
+	{ "tx-usecs", CMDL_S32, &coal_tx_usec_wanted,			\
+	  &__ecoal.tx_coalesce_usecs },					\
+	{ "tx-frames", CMDL_S32, &coal_tx_frames_wanted,		\
+	  &__ecoal.tx_max_coalesced_frames },				\
+	{ "tx-usecs-irq", CMDL_S32, &coal_tx_usec_irq_wanted,		\
+	  &__ecoal.tx_coalesce_usecs_irq },				\
+	{ "tx-frames-irq", CMDL_S32, &coal_tx_frames_irq_wanted,	\
+	  &__ecoal.tx_max_coalesced_frames_irq },			\
+	{ "rx-usecs-low", CMDL_S32, &coal_rx_usec_low_wanted,		\
+	  &__ecoal.rx_coalesce_usecs_low },				\
+	{ "rx-frames-low", CMDL_S32, &coal_rx_frames_low_wanted,	\
+	  &__ecoal.rx_max_coalesced_frames_low },			\
+	{ "tx-usecs-low", CMDL_S32, &coal_tx_usec_low_wanted,		\
+	  &__ecoal.tx_coalesce_usecs_low },				\
+	{ "tx-frames-low", CMDL_S32, &coal_tx_frames_low_wanted,	\
+	  &__ecoal.tx_max_coalesced_frames_low },			\
+	{ "rx-usecs-high", CMDL_S32, &coal_rx_usec_high_wanted,		\
+	  &__ecoal.rx_coalesce_usecs_high },				\
+	{ "rx-frames-high", CMDL_S32, &coal_rx_frames_high_wanted,	\
+	  &__ecoal.rx_max_coalesced_frames_high },			\
+	{ "tx-usecs-high", CMDL_S32, &coal_tx_usec_high_wanted,		\
+	  &__ecoal.tx_coalesce_usecs_high },				\
+	{ "tx-frames-high", CMDL_S32, &coal_tx_frames_high_wanted,	\
+	  &__ecoal.tx_max_coalesced_frames_high },			\
+}
+
 static int do_scoalesce(struct cmd_context *ctx)
 {
 	struct ethtool_coalesce ecoal;
 	int gcoalesce_changed = 0;
-	s32 coal_stats_wanted = -1;
-	int coal_adaptive_rx_wanted = -1;
-	int coal_adaptive_tx_wanted = -1;
-	s32 coal_sample_rate_wanted = -1;
-	s32 coal_pkt_rate_low_wanted = -1;
-	s32 coal_pkt_rate_high_wanted = -1;
-	s32 coal_rx_usec_wanted = -1;
-	s32 coal_rx_frames_wanted = -1;
-	s32 coal_rx_usec_irq_wanted = -1;
-	s32 coal_rx_frames_irq_wanted = -1;
-	s32 coal_tx_usec_wanted = -1;
-	s32 coal_tx_frames_wanted = -1;
-	s32 coal_tx_usec_irq_wanted = -1;
-	s32 coal_tx_frames_irq_wanted = -1;
-	s32 coal_rx_usec_low_wanted = -1;
-	s32 coal_rx_frames_low_wanted = -1;
-	s32 coal_tx_usec_low_wanted = -1;
-	s32 coal_tx_frames_low_wanted = -1;
-	s32 coal_rx_usec_high_wanted = -1;
-	s32 coal_rx_frames_high_wanted = -1;
-	s32 coal_tx_usec_high_wanted = -1;
-	s32 coal_tx_frames_high_wanted = -1;
-	struct cmdline_info cmdline_coalesce[] = {
-		{ "adaptive-rx", CMDL_BOOL, &coal_adaptive_rx_wanted,
-		  &ecoal.use_adaptive_rx_coalesce },
-		{ "adaptive-tx", CMDL_BOOL, &coal_adaptive_tx_wanted,
-		  &ecoal.use_adaptive_tx_coalesce },
-		{ "sample-interval", CMDL_S32, &coal_sample_rate_wanted,
-		  &ecoal.rate_sample_interval },
-		{ "stats-block-usecs", CMDL_S32, &coal_stats_wanted,
-		  &ecoal.stats_block_coalesce_usecs },
-		{ "pkt-rate-low", CMDL_S32, &coal_pkt_rate_low_wanted,
-		  &ecoal.pkt_rate_low },
-		{ "pkt-rate-high", CMDL_S32, &coal_pkt_rate_high_wanted,
-		  &ecoal.pkt_rate_high },
-		{ "rx-usecs", CMDL_S32, &coal_rx_usec_wanted,
-		  &ecoal.rx_coalesce_usecs },
-		{ "rx-frames", CMDL_S32, &coal_rx_frames_wanted,
-		  &ecoal.rx_max_coalesced_frames },
-		{ "rx-usecs-irq", CMDL_S32, &coal_rx_usec_irq_wanted,
-		  &ecoal.rx_coalesce_usecs_irq },
-		{ "rx-frames-irq", CMDL_S32, &coal_rx_frames_irq_wanted,
-		  &ecoal.rx_max_coalesced_frames_irq },
-		{ "tx-usecs", CMDL_S32, &coal_tx_usec_wanted,
-		  &ecoal.tx_coalesce_usecs },
-		{ "tx-frames", CMDL_S32, &coal_tx_frames_wanted,
-		  &ecoal.tx_max_coalesced_frames },
-		{ "tx-usecs-irq", CMDL_S32, &coal_tx_usec_irq_wanted,
-		  &ecoal.tx_coalesce_usecs_irq },
-		{ "tx-frames-irq", CMDL_S32, &coal_tx_frames_irq_wanted,
-		  &ecoal.tx_max_coalesced_frames_irq },
-		{ "rx-usecs-low", CMDL_S32, &coal_rx_usec_low_wanted,
-		  &ecoal.rx_coalesce_usecs_low },
-		{ "rx-frames-low", CMDL_S32, &coal_rx_frames_low_wanted,
-		  &ecoal.rx_max_coalesced_frames_low },
-		{ "tx-usecs-low", CMDL_S32, &coal_tx_usec_low_wanted,
-		  &ecoal.tx_coalesce_usecs_low },
-		{ "tx-frames-low", CMDL_S32, &coal_tx_frames_low_wanted,
-		  &ecoal.tx_max_coalesced_frames_low },
-		{ "rx-usecs-high", CMDL_S32, &coal_rx_usec_high_wanted,
-		  &ecoal.rx_coalesce_usecs_high },
-		{ "rx-frames-high", CMDL_S32, &coal_rx_frames_high_wanted,
-		  &ecoal.rx_max_coalesced_frames_high },
-		{ "tx-usecs-high", CMDL_S32, &coal_tx_usec_high_wanted,
-		  &ecoal.tx_coalesce_usecs_high },
-		{ "tx-frames-high", CMDL_S32, &coal_tx_frames_high_wanted,
-		  &ecoal.tx_max_coalesced_frames_high },
-	};
+	DECLARE_COALESCE_OPTION_VARS();
+	struct cmdline_info cmdline_coalesce[] = COALESCE_CMDLINE_INFO(ecoal);
 	int err, changed = 0;
 
 	parse_generic_cmdline(ctx, &gcoalesce_changed,
-- 
2.20.1

Re: [PATCH v3 2/6] ethtool: move cmdline_coalesce out of do_scoalesce

[Michal Kubecek]

On Fri, Mar 01, 2019 at 12:15:28AM -0800, Jeff Kirsher wrote:
> From: Nicholas Nunley <nicholas.d.nunley@intel.com>
> 
> Move the definition of cmdline_coalesce out of do_scoalesce and into a
> macro so it can be resused across functions.
> 
> No behavior change.
> 
> 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>

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

[Jeff Kirsher]

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>
---
 ethtool.8.in | 20 ++++++++++++++
 ethtool.c    | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+)

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 *);
-- 
2.20.1

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

[Michal Kubecek]

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

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

[Nunley, Nicholas D]

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

[PATCH v3 4/6] ethtool: support per-queue sub command --show-coalesce

[Jeff Kirsher]

From: Nicholas Nunley <nicholas.d.nunley@intel.com>

Get all masked queues' coalesce settings from kernel and dump them one by
one.

Example:

 $ sudo ./ethtool --per-queue eth5 queue_mask 0x11 --show-coalesce
 Queue: 0
 Adaptive RX: off  TX: off
 stats-block-usecs: 0
 sample-interval: 0
 pkt-rate-low: 0
 pkt-rate-high: 0

 rx-usecs: 222
 rx-frames: 0
 rx-usecs-irq: 0
 rx-frames-irq: 256

 tx-usecs: 222
 tx-frames: 0
 tx-usecs-irq: 0
 tx-frames-irq: 256

 rx-usecs-low: 0
 rx-frame-low: 0
 tx-usecs-low: 0
 tx-frame-low: 0

 rx-usecs-high: 0
 rx-frame-high: 0
 tx-usecs-high: 0
 tx-frame-high: 0

 Queue: 4
 Adaptive RX: off  TX: off
 stats-block-usecs: 0
 sample-interval: 0
 pkt-rate-low: 0
 pkt-rate-high: 0

 rx-usecs: 222
 rx-frames: 0
 rx-usecs-irq: 0
 rx-frames-irq: 256

 tx-usecs: 222
 tx-frames: 0
 tx-usecs-irq: 0
 tx-frames-irq: 256

 rx-usecs-low: 0
 rx-frame-low: 0
 tx-usecs-low: 0
 tx-frame-low: 0

 rx-usecs-high: 0
 rx-frame-high: 0
 tx-usecs-high: 0
 tx-frame-high: 0

Based on patch by Kan Liang <kan.liang@intel.com>

Signed-off-by: Nicholas Nunley <nicholas.d.nunley@intel.com>
---
 ethtool.8.in |  2 +-
 ethtool.c    | 68 +++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index 10f24db..b4e240e 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
-Sub command to apply.
+Sub command to apply. 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 5f72741..1abf7c0 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1410,6 +1410,31 @@ 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, int n_queues)
+{
+	struct ethtool_coalesce *ecoal;
+	int i, idx = 0;
+
+	ecoal = (struct ethtool_coalesce *)(per_queue_opt + 1);
+	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(ecoal + idx);
+				idx++;
+			}
+			mask = mask >> 1;
+			queue++;
+		}
+		if (idx == n_queues)
+			break;
+	}
+}
+
 struct feature_state {
 	u32 off_flags;
 	struct ethtool_gfeatures features;
@@ -5248,7 +5273,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",
+	{ "-Q|--per-queue", 1, do_perqueue, "Apply 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" },
@@ -5315,8 +5341,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;
 
@@ -5367,9 +5417,21 @@ 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, n_queues);
+		free(per_queue_opt);
+	} else {
+		perror("The subcommand is not supported yet");
+		return -EOPNOTSUPP;
+	}
 
-	return -EOPNOTSUPP;
+	return 0;
 }
 
 int main(int argc, char **argp)
-- 
2.20.1

Re: [PATCH v3 4/6] ethtool: support per-queue sub command --show-coalesce

[Michal Kubecek]

On Fri, Mar 01, 2019 at 12:15:30AM -0800, Jeff Kirsher wrote:
> From: Nicholas Nunley <nicholas.d.nunley@intel.com>
> 
> Get all masked queues' coalesce settings from kernel and dump them one by
> one.
> 
> Example:
> 
>  $ sudo ./ethtool --per-queue eth5 queue_mask 0x11 --show-coalesce
>  Queue: 0
>  Adaptive RX: off  TX: off
>  stats-block-usecs: 0
>  sample-interval: 0
>  pkt-rate-low: 0
>  pkt-rate-high: 0
> 
>  rx-usecs: 222
>  rx-frames: 0
>  rx-usecs-irq: 0
>  rx-frames-irq: 256
> 
>  tx-usecs: 222
>  tx-frames: 0
>  tx-usecs-irq: 0
>  tx-frames-irq: 256
> 
>  rx-usecs-low: 0
>  rx-frame-low: 0
>  tx-usecs-low: 0
>  tx-frame-low: 0
> 
>  rx-usecs-high: 0
>  rx-frame-high: 0
>  tx-usecs-high: 0
>  tx-frame-high: 0
> 
>  Queue: 4
>  Adaptive RX: off  TX: off
>  stats-block-usecs: 0
>  sample-interval: 0
>  pkt-rate-low: 0
>  pkt-rate-high: 0
> 
>  rx-usecs: 222
>  rx-frames: 0
>  rx-usecs-irq: 0
>  rx-frames-irq: 256
> 
>  tx-usecs: 222
>  tx-frames: 0
>  tx-usecs-irq: 0
>  tx-frames-irq: 256
> 
>  rx-usecs-low: 0
>  rx-frame-low: 0
>  tx-usecs-low: 0
>  tx-frame-low: 0
> 
>  rx-usecs-high: 0
>  rx-frame-high: 0
>  tx-usecs-high: 0
>  tx-frame-high: 0
> 
> 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>

> ---
>  ethtool.8.in |  2 +-
>  ethtool.c    | 68 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/ethtool.8.in b/ethtool.8.in
> index 10f24db..b4e240e 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
...
> @@ -5248,7 +5273,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",
> +	{ "-Q|--per-queue", 1, do_perqueue, "Apply per-queue command."
> +	  "The supported sub commands include --show-coalesce",

Nitpick: missing space: "...per-queue command.The supported..."

Michal Kubecek

[PATCH v3 5/6] ethtool: support per-queue sub command --coalesce

[Jeff Kirsher]

From: Nicholas Nunley <nicholas.d.nunley@intel.com>

This patch adds the ability to configure the coalesce settings from
do_scoalesce on a per-queue basis.

For each masked queue the current settings are read, modified, and written
back to the kernel.

Example:

 $ sudo ./ethtool --per-queue eth5 queue_mask 0x1 --coalesce
 rx-usecs 10 tx-usecs 5
 $ sudo ./ethtool --per-queue eth5 queue_mask 0x1 --show-coalesce

 Queue: 0
 Adaptive RX: on  TX: on
 stats-block-usecs: 0
 sample-interval: 0
 pkt-rate-low: 0
 pkt-rate-high: 0

 rx-usecs: 10
 rx-frames: 0
 rx-usecs-irq: 0
 rx-frames-irq: 256

 tx-usecs: 5
 tx-frames: 0
 tx-usecs-irq: 0
 tx-frames-irq: 256

 rx-usecs-low: 0
 rx-frame-low: 0
 tx-usecs-low: 0
 tx-frame-low: 0

 rx-usecs-high: 0
 rx-frame-high: 0
 tx-usecs-high: 0
 tx-frame-high: 0

Based on patch by Kan Liang <kan.liang@intel.com>

Signed-off-by: Nicholas Nunley <nicholas.d.nunley@intel.com>
---
 ethtool.8.in |  3 ++-
 ethtool.c    | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index b4e240e..2f788f3 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -1153,7 +1153,8 @@ 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. The supported sub commands include --show-coalesce.
+Sub command to apply. The supported sub commands include --show-coalesce and
+--coalesce.
 .RE
 .SH BUGS
 Not supported (in part or whole) on all network drivers.
diff --git a/ethtool.c b/ethtool.c
index 1abf7c0..2296578 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5274,7 +5274,7 @@ static const struct option {
 	{ "--set-fec", 1, do_sfec, "Set FEC settings",
 	  "		[ encoding auto|off|rs|baser [...]]\n"},
 	{ "-Q|--per-queue", 1, do_perqueue, "Apply per-queue command."
-	  "The supported sub commands include --show-coalesce",
+	  "The supported sub commands include --show-coalesce, --coalesce",
 	  "             [queue_mask %x] SUB_COMMAND\n"},
 	{ "-h|--help", 0, show_usage, "Show this help" },
 	{ "--version", 0, do_version, "Show version number" },
@@ -5364,6 +5364,57 @@ get_per_queue_coalesce(struct cmd_context *ctx, __u32 *queue_mask, int n_queues)
 	return per_queue_opt;
 }
 
+static void set_per_queue_coalesce(struct cmd_context *ctx,
+				   struct ethtool_per_queue_op *per_queue_opt,
+				   int n_queues)
+{
+	struct ethtool_coalesce ecoal;
+	DECLARE_COALESCE_OPTION_VARS();
+	struct cmdline_info cmdline_coalesce[] = COALESCE_CMDLINE_INFO(ecoal);
+	__u32 *queue_mask = per_queue_opt->queue_mask;
+	struct ethtool_coalesce *ecoal_q;
+	int gcoalesce_changed = 0;
+	int i, idx = 0;
+
+	parse_generic_cmdline(ctx, &gcoalesce_changed,
+			      cmdline_coalesce, ARRAY_SIZE(cmdline_coalesce));
+
+	ecoal_q = (struct ethtool_coalesce *)(per_queue_opt + 1);
+	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) {
+				int changed = 0;
+
+				memcpy(&ecoal, ecoal_q + idx,
+				       sizeof(struct ethtool_coalesce));
+				do_generic_set(cmdline_coalesce,
+					       ARRAY_SIZE(cmdline_coalesce),
+					       &changed);
+				if (!changed)
+					fprintf(stderr,
+						"Queue %d, no coalesce parameters changed\n",
+						queue);
+				memcpy(ecoal_q + idx, &ecoal,
+				       sizeof(struct ethtool_coalesce));
+				idx++;
+			}
+			mask = mask >> 1;
+			queue++;
+		}
+		if (idx == n_queues)
+			break;
+	}
+
+	per_queue_opt->cmd = ETHTOOL_PERQUEUE;
+	per_queue_opt->sub_command = ETHTOOL_SCOALESCE;
+
+	if (send_ioctl(ctx, per_queue_opt))
+		perror("Cannot set device per queue parameters");
+}
+
 static int do_perqueue(struct cmd_context *ctx)
 {
 	struct ethtool_per_queue_op *per_queue_opt;
@@ -5426,6 +5477,17 @@ static int do_perqueue(struct cmd_context *ctx)
 		}
 		dump_per_queue_coalesce(per_queue_opt, queue_mask, n_queues);
 		free(per_queue_opt);
+	} else if (strstr(args[i].opts, "--coalesce") != NULL) {
+		ctx->argc--;
+		ctx->argp++;
+		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;
+		}
+		set_per_queue_coalesce(ctx, per_queue_opt, n_queues);
+		free(per_queue_opt);
 	} else {
 		perror("The subcommand is not supported yet");
 		return -EOPNOTSUPP;
-- 
2.20.1

Re: [PATCH v3 5/6] ethtool: support per-queue sub command --coalesce

[Michal Kubecek]

On Fri, Mar 01, 2019 at 12:15:31AM -0800, Jeff Kirsher wrote:
> From: Nicholas Nunley <nicholas.d.nunley@intel.com>
> 
> This patch adds the ability to configure the coalesce settings from
> do_scoalesce on a per-queue basis.
> 
> For each masked queue the current settings are read, modified, and written
> back to the kernel.
> 
> Example:
> 
>  $ sudo ./ethtool --per-queue eth5 queue_mask 0x1 --coalesce
>  rx-usecs 10 tx-usecs 5
>  $ sudo ./ethtool --per-queue eth5 queue_mask 0x1 --show-coalesce
> 
>  Queue: 0
>  Adaptive RX: on  TX: on
>  stats-block-usecs: 0
>  sample-interval: 0
>  pkt-rate-low: 0
>  pkt-rate-high: 0
> 
>  rx-usecs: 10
>  rx-frames: 0
>  rx-usecs-irq: 0
>  rx-frames-irq: 256
> 
>  tx-usecs: 5
>  tx-frames: 0
>  tx-usecs-irq: 0
>  tx-frames-irq: 256
> 
>  rx-usecs-low: 0
>  rx-frame-low: 0
>  tx-usecs-low: 0
>  tx-frame-low: 0
> 
>  rx-usecs-high: 0
>  rx-frame-high: 0
>  tx-usecs-high: 0
>  tx-frame-high: 0
> 
> 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>

[PATCH v3 6/6] ethtool: fix up dump_coalesce output to match actual option names

[Jeff Kirsher]

From: Nicholas Nunley <nicholas.d.nunley@intel.com>

When the coalesce settings are printed with --show-coalesce a few of the
option names lack the pluralization that is present in the man page and
usage info, but are otherwise identical.

This inconsistency could lead to some confusion if a user attempts to set
the coalesce settings by matching the output they see from --show-coalesce,
so fix this.

Signed-off-by: Nicholas Nunley <nicholas.d.nunley@intel.com>
---
 ethtool.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 2296578..8e20616 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1373,14 +1373,14 @@ static int dump_coalesce(const struct ethtool_coalesce *ecoal)
 		"tx-frames-irq: %u\n"
 		"\n"
 		"rx-usecs-low: %u\n"
-		"rx-frame-low: %u\n"
+		"rx-frames-low: %u\n"
 		"tx-usecs-low: %u\n"
-		"tx-frame-low: %u\n"
+		"tx-frames-low: %u\n"
 		"\n"
 		"rx-usecs-high: %u\n"
-		"rx-frame-high: %u\n"
+		"rx-frames-high: %u\n"
 		"tx-usecs-high: %u\n"
-		"tx-frame-high: %u\n"
+		"tx-frames-high: %u\n"
 		"\n",
 		ecoal->stats_block_coalesce_usecs,
 		ecoal->rate_sample_interval,
-- 
2.20.1

Re: [PATCH v3 6/6] ethtool: fix up dump_coalesce output to match actual option names

[Michal Kubecek]

On Fri, Mar 01, 2019 at 12:15:32AM -0800, Jeff Kirsher wrote:
> From: Nicholas Nunley <nicholas.d.nunley@intel.com>
> 
> When the coalesce settings are printed with --show-coalesce a few of the
> option names lack the pluralization that is present in the man page and
> usage info, but are otherwise identical.
> 
> This inconsistency could lead to some confusion if a user attempts to set
> the coalesce settings by matching the output they see from --show-coalesce,
> so fix this.
> 
> Signed-off-by: Nicholas Nunley <nicholas.d.nunley@intel.com>

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

Re: [PATCH v3 1/6] ethtool: move option parsing related code into function

[Michal Kubecek]

On Fri, Mar 01, 2019 at 12:15:27AM -0800, Jeff Kirsher wrote:
> From: Nicholas Nunley <nicholas.d.nunley@intel.com>
> 
> Move option parsing code into find_option function.
> 
> No behavior changes.
> 
> 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>

Re: [PATCH v3 1/6] ethtool: move option parsing related code into function

[John W. Linville]

On Fri, Mar 01, 2019 at 12:15:27AM -0800, Jeff Kirsher wrote:
> From: Nicholas Nunley <nicholas.d.nunley@intel.com>
> 
> Move option parsing code into find_option function.
> 
> No behavior changes.
> 
> Based on patch by Kan Liang <kan.liang@intel.com>
> 
> Signed-off-by: Nicholas Nunley <nicholas.d.nunley@intel.com>

Series queued for next release -- thanks!

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.