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

[PATCH v2 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>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 ethtool.c | 49 +++++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 2f7e96b..8b7c224 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5265,6 +5265,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 *);
@@ -5284,24 +5307,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 v2 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 reused 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>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 ethtool.c | 142 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 74 insertions(+), 68 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 8b7c224..af266c5 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2098,78 +2098,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

[PATCH v2 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 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

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>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 ethtool.8.in |  20 ++++++++++
 ethtool.c    | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+)

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

[PATCH v2 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 --set-perqueue-command 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>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 ethtool.8.in |  2 +-
 ethtool.c    | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 3 deletions(-)

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;
 }
-- 
2.20.1

[PATCH v2 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 --set-perqueue-command eth5 queue_mask 0x1 --coalesce
 rx-usecs 10 tx-usecs 5
 $ sudo ./ethtool --set-perqueue-command 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>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 ethtool.8.in |  3 ++-
 ethtool.c    | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index 71bb962..c7202e8 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
-Sets the sub command. The supported sub commands include --show-coalesce.
+Sets the sub command. 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 9a1b83b..01bdaf1 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5269,7 +5269,7 @@ static const struct option {
 	{ "--set-fec", 1, do_sfec, "Set FEC settings",
 	  "		[ encoding auto|off|rs|baser [...]]\n"},
 	{ "--set-perqueue-command", 1, do_perqueue, "Set 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" },
@@ -5397,6 +5397,53 @@ 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)
+{
+	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;
+	char *addr = (char *)per_queue_opt + sizeof(*per_queue_opt);
+	int gcoalesce_changed = 0;
+	int i;
+
+	parse_generic_cmdline(ctx, &gcoalesce_changed,
+			      cmdline_coalesce, ARRAY_SIZE(cmdline_coalesce));
+
+	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, addr,
+				       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(addr, &ecoal,
+				       sizeof(struct ethtool_coalesce));
+				addr += sizeof(struct ethtool_coalesce);
+			}
+			mask = mask >> 1;
+			queue++;
+		}
+	}
+
+	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;
@@ -5447,6 +5494,17 @@ static int do_perqueue(struct cmd_context *ctx)
 		}
 		dump_per_queue_coalesce(per_queue_opt, queue_mask);
 		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);
+		free(per_queue_opt);
 	} else {
 		perror("The subcommand is not supported yet");
 		return -EOPNOTSUPP;
-- 
2.20.1

[PATCH v2 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>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 ethtool.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 01bdaf1..03e5008 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1372,14 +1372,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 v2 3/6] ethtool: introduce new ioctl for per-queue settings

[Michal Kubecek]

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

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

[Michal Kubecek]

On Tue, Feb 05, 2019 at 04:01:01PM -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>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  ethtool.c | 49 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 2f7e96b..8b7c224 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -5265,6 +5265,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 *);
> @@ -5284,24 +5307,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();

After hiding the loop into find_option() helper, you can put the
following few lines into else branch and get rid of the goto.

Michal Kubecek

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

[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

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

[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

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

[Michal Kubecek]

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

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

[Nunley, Nicholas D]

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

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

[Nunley, Nicholas D]

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

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

[Nunley, Nicholas D]

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

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

[Nunley, Nicholas D]

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

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

[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

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

[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

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

[Willem de Bruijn]

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