RSS improvements + new speeds

From: Eyal Grossman <[email protected]>

This covers kernel changes up to:

commit 892311f66f2411b813ca631009356891a0c2b0a1

Author: Eyal Perry <[email protected]>
Date:   Tue Dec 2 18:12:10 2014 +0200

ethtool: Support for configurable RSS hash function

Signed-off-by Eyal Grossman <[email protected]>

Signed-off-by Amir Vadai <[email protected]>

---
 ethtool-copy.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 8 deletions(-)

ethtool-copy.h +0 -0

index 61b78fc..5f66d9c 100644

Quoted 28 lines · Amir Vadai

From: Eyal Grossman <[email protected]>

This covers kernel changes up to:

commit 892311f66f2411b813ca631009356891a0c2b0a1

Author: Eyal Perry <[email protected]>
Date:   Tue Dec 2 18:12:10 2014 +0200

ethtool: Support for configurable RSS hash function

Signed-off-by Eyal Grossman <[email protected]>

Signed-off-by Amir Vadai <[email protected]>

---
 ethtool-copy.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 8 deletions(-)

ethtool-copy.h +2 -2

index 61b78fc..5f66d9c 100644
@@ -10,8 +10,8 @@
  * Portions Copyright (C) Sun Microsystems 2008
  */
 
-#ifndef _LINUX_ETHTOOL_H
-#define _LINUX_ETHTOOL_H
+#ifndef _UAPI_LINUX_ETHTOOL_H
+#define _UAPI_LINUX_ETHTOOL_H

[...]

You've copied from include/uapi/linux/ethtool.h, which is not quite right. The include/uapi directory contains definitions shared with userland, but the headers still need some processing (like removing the _UAPI prefix from header guards). You should use 'make headers_install' to generate the userland version in usr/include/linux/ethtool.h. Anyway, I've done that myself based on today's net tree.

Ben.

--
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

Quoted 1 line · Amir Vadai

From: Eyal Grossman <[email protected]>

You've copied from include/uapi/linux/ethtool.h, which is not quite right. The include/uapi directory contains definitions shared with userland, but the headers still need some processing (like removing the _UAPI prefix from header guards). You should use 'make headers_install' to generate the userland version in usr/include/linux/ethtool.h.

Anyway, I've done that myself based on today's net tree.

Ben.

--
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

Quoted 2 lines · Ben Hutchings

Quoted 1 line · Amir Vadai

From: Eyal Grossman <[email protected]>

[...]

Quoted 6 lines

You've copied from include/uapi/linux/ethtool.h, which is not quite right. The include/uapi directory contains definitions shared with userland, but the headers still need some processing (like removing the _UAPI prefix from header guards). You should use 'make headers_install' to generate the userland version in usr/include/linux/ethtool.h.

Sorry, didn't know.

Quoted 1 line

Anyway, I've done that myself based on today's net tree.

Is it in your git [1] ? Because I couldn't find the updated version there.

[1] - git://git.kernel.org/pub/scm/network/ethtool/ethtool.git

From: Eyal Grossman <[email protected]>

Added the following missing advertised speed modes:
- ADVERTISED_10000baseT_Full
- ADVERTISED_56000baseKR4_Full
- ADVERTISED_56000baseCR4_Full
- ADVERTISED_56000baseSR4_Full
- ADVERTISED_56000baseLR4_Full
- ADVERTISED_10000baseKX4_Full

In order to reduce code duplication we added a macro to ALL_ADVERTISED_FLAGS in line 88 ALL_ADVERTISED_MODES, in addition the changed we made added speed that were missing from ALL_ADVERTISED_MODES
(e.g. ADVERTISED_10000baseKX4_Full).

Added ADVERTISED_10000baseR_FEC to mode_defs.

Signed-off-by Eyal Grossman <[email protected]>

Signed-off-by Amir Vadai <[email protected]>

---

ethtool.c +4 -0

 		{ 0, ADVERTISED_20000baseMLD2_Full, "20000baseMLD2/Full" },
 		{ 0, ADVERTISED_20000baseKR2_Full,  "20000baseKR2/Full" },
 		{ 0, ADVERTISED_40000baseKR4_Full,  "40000baseKR4/Full" },
 		{ 0, ADVERTISED_40000baseCR4_Full,  "40000baseCR4/Full" },
 		{ 0, ADVERTISED_40000baseSR4_Full,  "40000baseSR4/Full" },
 		{ 0, ADVERTISED_40000baseLR4_Full,  "40000baseLR4/Full" },
+		{ 0, ADVERTISED_56000baseKR4_Full,  "56000baseKR4/Full" },
+		{ 0, ADVERTISED_56000baseCR4_Full,  "56000baseCR4/Full" },
+		{ 0, ADVERTISED_56000baseSR4_Full,  "56000baseSR4/Full" },
+		{ 0, ADVERTISED_56000baseLR4_Full,  "56000baseLR4/Full" },
 	};
 	int indent;
 	int did1, new_line_pend, i;

Quoted 10 lines · Amir Vadai

From: Eyal Grossman <[email protected]>

Added the following missing advertised speed modes:
- ADVERTISED_10000baseT_Full
- ADVERTISED_56000baseKR4_Full
- ADVERTISED_56000baseCR4_Full
- ADVERTISED_56000baseSR4_Full
- ADVERTISED_56000baseLR4_Full
- ADVERTISED_10000baseKX4_Full

Quoted 5 lines

In order to reduce code duplication we added a macro to ALL_ADVERTISED_FLAGS in line 88 ALL_ADVERTISED_MODES, in addition the changed we made added speed that were missing from ALL_ADVERTISED_MODES
(e.g. ADVERTISED_10000baseKX4_Full).

Well spotted.

Quoted 5 lines

Added ADVERTISED_10000baseR_FEC to mode_defs.

Signed-off-by Eyal Grossman <[email protected]>

Signed-off-by Amir Vadai <[email protected]>

---

[...]

ethtool.c dump_link_caps(const char *prefix, const char *an_prefix, u32 mask, · line 519

Quoted 6 lines

ethtool.c +1 -1

@@ -527,15 +516,20 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
 		{ 1, ADVERTISED_1000baseT_Full,     "1000baseT/Full" },
 		{ 0, ADVERTISED_1000baseKX_Full,    "1000baseKX/Full" },
 		{ 0, ADVERTISED_2500baseX_Full,     "2500baseX/Full" },
-		{ 0, ADVERTISED_10000baseT_Full,    "10000baseT/Full" },
+		{ 1, ADVERTISED_10000baseT_Full,    "10000baseT/Full" },

Why should this be on the same line as 2500baseX/Full?

ethtool.c dump_link_caps(const char *prefix, const char *an_prefix, u32 mask, · line 522

Quoted 3 lines

ethtool.c +1 -0

 		{ 0, ADVERTISED_10000baseKX4_Full,  "10000baseKX4/Full" },
 		{ 0, ADVERTISED_10000baseKR_Full,   "10000baseKR/Full" },
+		{ 0, ADVERTISED_10000baseR_FEC,     "10000baseR/FEC" },

This is not a link mode, it's a separate capability that applies to all 10GBASE-R modes. It's also meaningful for 40G and 100G modes despite the way we've named the flags. Please put it in a separate section below the link modes.

Review

Quoted 3 lines

(quoted diff) +1 -0

 		{ 0, ADVERTISED_10000baseKX4_Full,  "10000baseKX4/Full" },
 		{ 0, ADVERTISED_10000baseKR_Full,   "10000baseKR/Full" },
+		{ 0, ADVERTISED_10000baseR_FEC,     "10000baseR/FEC" },

This is not a link mode, it's a separate capability that applies to all 10GBASE-R modes. It's also meaningful for 40G and 100G modes despite the way we've named the flags. Please put it in a separate section below the link modes.

From: Eyal Grossman <[email protected]>

Added to ethtool return code(rc != 0) when an error occurs after send_ioctl has fails.

Signed-off-by Eyal Grossman <[email protected]>

Signed-off-by Amir Vadai <[email protected]>

ethtool.c +1 -0

@@ -2654,17 +2661,20 @@ static int do_sset(struct cmd_context *ctx)
 		err = send_ioctl(ctx, &edata);
 		if (err < 0) {
 			perror("Cannot get msglvl");
+			status += err;
 		} else {
 			edata.cmd = ETHTOOL_SMSGLVL;
 			edata.data = ((edata.data & ~msglvl_mask) |
 				      msglvl_wanted);
 			err = send_ioctl(ctx, &edata);

ethtool.c +0 -0

 }
 
 static int do_gregs(struct cmd_context *ctx)

From: Eyal Perry <[email protected]>

When indirection table size is not a multiple of 8, a new line is missing at the last row of the table.

In addition, make a distinction between the attribute name and its value by adding an indentation on the beginning of each line which contains values.

We only print a new-line after every 8 entries, but we need one at the end of the table even if the table size is not a multiple of 8.

Reported-by Amir Vadai <[email protected]> Signed-off-by: Ben

Hutchings <[email protected]> --- This is what I've applied - hope it works for you as I don't have a test case.

From: Eyal Perry <[email protected]>

This ethtool patch adds support to set and get the current RSS hash function for the device through through the new hfunc mask field in the ethtool_rxfh struct. Kernel supported hash function names are queried with ETHTOOL_GSTRINGS - each string is corresponding with a bit in hfunc mask according to its index in the string-set.

Signed-off-by Eyal Perry <[email protected]>

Signed-off-by Amir Vadai <[email protected]>

---
 ethtool.8.in |  7 +++++++
 ethtool.c    | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 58 insertions(+), 5 deletions(-)

ethtool.8.in +2 -0

index ae56293..bdc77e0 100644
@@ -297,6 +297,8 @@ ethtool \- query or control network driver and hardware settings
 .BI weight\  W0
 .IR W1
 .RB ...\ ]
+.RB [ hfunc
+.IR FUNC ]
 .HP
 .B ethtool \-f|\-\-flash
 .I devname file

ethtool.8.in +1 -0

+.TP
 .BI equal\  N
 Sets the receive flow hash indirection table to spread flows evenly

between the first \fIN\fR receive queues.

ethtool.c +8 -1

index c2f4164..c584333 100644
@@ -3176,10 +3176,11 @@ static int do_grxfh(struct cmd_context *ctx)
 {
 	struct ethtool_rxfh rss_head = {0};
 	struct ethtool_rxnfc ring_count;
+	struct ethtool_gstrings *hfuncs;
 	struct ethtool_rxfh *rss;
 	u32 i, indir_bytes;
 	char *hkey;
-	int err;
+	int err, cur_len, max_len = 0;
 
 	ring_count.cmd = ETHTOOL_GRXRINGS;
 	err = send_ioctl(ctx, &ring_count);

@@ -3231,6 +3232,23 @@ static int do_grxfh(struct cmd_context *ctx)
 			printf("%02x:", (u8) hkey[i]);
 	}
 
+	printf("RSS hash function:\n");
+	if (!rss->hfunc) {
+		printf("    Operation not supported\n");
+		goto out;
+	}
+	hfuncs = get_stringset(ctx, ETH_SS_RSS_HASH_FUNCS, 0, 1);

ethtool.c +1 -0

+out:
 	free(rss);
 	return 0;
 }

ethtool.c +2 -1

 	if (fill_indir_table(&rss->indir_size, rss->rss_config, rxfhindir_equal,
 			     rxfhindir_weight, num_weights)) {

@@ -4119,7 +4164,8 @@ static const struct option {
 	{ "-X|--set-rxfh-indir|--rxfh", 1, do_srxfh,
 	  "Set Rx flow hash indirection and/or hash key",
 	  "		[ equal N | weight W0 W1 ... ]\n"
-	  "		[ hkey %x:%x:%x:%x:%x:.... ]\n" },
+	  "		[ hkey %x:%x:%x:%x:%x:.... ]\n"
+	  "		[ hfunc FUNC ]\n"},
 	{ "-f|--flash", 1, do_flash,
 	  "Flash firmware image from the specified file to a region on the device",
 	  "               FILENAME [ REGION-NUMBER-TO-FLASH ]\n" },

Quoted 30 lines · Amir Vadai

From: Eyal Perry <[email protected]>

This ethtool patch adds support to set and get the current RSS hash function for the device through through the new hfunc mask field in the ethtool_rxfh struct. Kernel supported hash function names are queried with ETHTOOL_GSTRINGS - each string is corresponding with a bit in hfunc mask according to its index in the string-set.

Signed-off-by Eyal Perry <[email protected]>

Signed-off-by Amir Vadai <[email protected]>

---
 ethtool.8.in |  7 +++++++
 ethtool.c    | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 58 insertions(+), 5 deletions(-)

ethtool.8.in +2 -0

index ae56293..bdc77e0 100644
@@ -297,6 +297,8 @@ ethtool \- query or control network driver and hardware settings
 .BI weight\  W0
 .IR W1
 .RB ...\ ]
+.RB [ hfunc
+.IR FUNC ]
 .HP
 .B ethtool \-f|\-\-flash
 .I devname file

@@ -796,6 +798,11 @@ Sets RSS hash key of the specified network device. RSS hash key should be of dev
 Hash key format must be in xx:yy:zz:aa:bb:cc format meaning both the nibbles of a byte should be mentioned

ethtool.8.in Sets RSS hash key of the specified network device. RSS hash key should be of dev

Quoted 5 lines

ethtool.8.in +3 -0

 even if a nibble is zero.
 .TP
+.BI hfunc
+Sets RSS hash function of the specified network device. Requested hash function
+should be supported by the kernel and the device.

I think the second sentence is unneeded - obviously requesting an unsupported hash function will fail.

ethtool.8.in Sets RSS hash key of the specified network device. RSS hash key should be of dev · line 804

Quoted 2 lines

ethtool.8.in +0 -0

 List of RSS hash functions

'A list of ...'

ethtool.8.in Sets RSS hash key of the specified network device. RSS hash key should be of dev · line 804

Quoted 2 lines

ethtool.8.in +1 -0

+which kernel supports is shown as a part of the --show-rxfh comand output.

'... which the device supports ...' (since this depends on the hardware and driver as well as the kernel).

Quoted 38 lines

ethtool.8.in +1 -0

+.TP
 .BI equal\  N
 Sets the receive flow hash indirection table to spread flows evenly

between the first \fIN\fR receive queues.

ethtool.c +14 -1

index c2f4164..c584333 100644
@@ -3176,10 +3176,11 @@ static int do_grxfh(struct cmd_context *ctx)
 {
      struct ethtool_rxfh rss_head = {0};
      struct ethtool_rxnfc ring_count;
+     struct ethtool_gstrings *hfuncs;
      struct ethtool_rxfh *rss;
      u32 i, indir_bytes;
      char *hkey;
-     int err;
+     int err, cur_len, max_len = 0;
 
      ring_count.cmd = ETHTOOL_GRXRINGS;
      err = send_ioctl(ctx, &ring_count);

@@ -3231,6 +3232,23 @@ static int do_grxfh(struct cmd_context *ctx)
                      printf("%02x:", (u8) hkey[i]);
      }
 
+     printf("RSS hash function:\n");
+     if (!rss->hfunc) {
+             printf("    Operation not supported\n");
+             goto out;
+     }
+     hfuncs = get_stringset(ctx, ETH_SS_RSS_HASH_FUNCS, 0, 1);
+     for (i = 0; i < hfuncs->len; i++) {
+             cur_len = strlen((const char *)hfuncs->data +
+                              i * ETH_GSTRING_LEN);
+             if (cur_len > max_len)
+                     max_len = cur_len;
+     }

ethtool.c static int do_grxfh(struct cmd_context *ctx) · line 3250

Quoted 4 lines

ethtool.c +4 -0

+     for (i = 0; i < hfuncs->len; i++)
+             printf("    %-*s: %s\n", max_len,
+                    (const char *)hfuncs->data + i * ETH_GSTRING_LEN,
+                    (rss->hfunc & (1 << i)) ? "on" : "off");

I'm puzzled by this. Do you remember why the hash functions were given flag numbers rather than serial numbers? Would it ever make sense to have multiple hash functions enabled?

If only one hash function can be enabled (which seems to be true at least for current drivers) then this only needs to print the one hash function that is enabled. (But then, the user needs to be able to list the available hash functions somehow.)

If multiple hash functions can be enabled, then we should support that in do_srxfh(), don't we?

ethtool.c static int do_grxfh(struct cmd_context *ctx) · line 3254

Quoted 5 lines

ethtool.c +1 -0

+out:
      free(rss);
      return 0;
 }

[...]

ethtool.c static int do_srxfh(struct cmd_context *ctx) · line 3457

Quoted 5 lines

ethtool.c +1 -0

@@ -3410,6 +3454,7 @@ static int do_srxfh(struct cmd_context *ctx)
      rss->cmd = ETHTOOL_SRSSH;
      rss->indir_size = rss_head.indir_size;
      rss->key_size = rss_head.key_size;
+     rss->hfunc = req_hfunc ? req_hfunc : 0;

That's a complicated way to write '= req_hfunc;'.

Ben.

ethtool.c static const struct option { · line 4171

Quoted 12 lines

ethtool.c +2 -1

      if (fill_indir_table(&rss->indir_size, rss->rss_config, rxfhindir_equal,
                           rxfhindir_weight, num_weights)) {

@@ -4119,7 +4164,8 @@ static const struct option {
      { "-X|--set-rxfh-indir|--rxfh", 1, do_srxfh,
        "Set Rx flow hash indirection and/or hash key",
        "             [ equal N | weight W0 W1 ... ]\n"
-       "             [ hkey %x:%x:%x:%x:%x:.... ]\n" },
+       "             [ hkey %x:%x:%x:%x:%x:.... ]\n"
+       "             [ hfunc FUNC ]\n"},
      { "-f|--flash", 1, do_flash,
        "Flash firmware image from the specified file to a region on the device",
        "               FILENAME [ REGION-NUMBER-TO-FLASH ]\n" },

--
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

Hi Ben,

This patchset contains some features and small bug fixes.
A patch by Eyal Perry to enable setting RSS hash function, kernel support was added in commit 892311f ("ethtool: Support for configurable RSS hash function").
Eyal Grossman added some missing advertised speeds and fixed some issues in this area of code.

Patches were tested and applied on top of commit 4614098 ("Release version 3.16.")

Thanks,
Amir

Eyal Grossman (3):
ethtool-copy.h: sync with net
ethtool: Add missing Advertised speeds
ethtool: Return bad status when send_ioctl fails

Eyal Perry (2):
ethtool: Prettify RX flow hash indirection table print
ethtool: Support for configurable RSS hash function

 ethtool-copy.h |  67 +++++++++++++++++++++++++++----
 ethtool.8.in   |   7 ++++
 ethtool.c      | 122 ++++++++++++++++++++++++++++++++++++++++-----------------
 3 files changed, 152 insertions(+), 44 deletions(-)

--
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 8 lines · Amir Vadai

Hi Ben,

This patchset contains some features and small bug fixes.
A patch by Eyal Perry to enable setting RSS hash function, kernel support was added in commit 892311f ("ethtool: Support for configurable RSS hash function").
Eyal Grossman added some missing advertised speeds and fixed some issues in this area of code.

Ben, Amir, where this series stands?