(repost for 2020y) inconsistency of ethtool feature names for get vs. set

(repost for 2020y) inconsistency of ethtool feature names for get vs. set

[Konstantin Kharlamov]

I just started using the -K and -k options, and I was very confused why nothing I was trying worked. I followed the documentation, which says that there're predefined options, (e.g. gso, gro) but kernel may also define others. And according to the `-k` output, sure they were, like "generic-segmentation-offload" and "large-receive-offload"! Well, at least I thought so.

Researching the internet led me to the old report of this behavior. So, it was already discussed in 2014, someone got a patch to fix this, and everyone was happy. The patch has never made it into the discussion though.

I figured, the project has no bugtracker, so once discussion stopped, a report is lost. I guess, 6 years is a big enough timespan to consider a discussion stopped, so I'm bringing it up again. What follows is a copy of the report here https://www.spinics.net/lists/netdev/msg264222.html

P.S.: please add me to CC, I'm not subscribed to the list.

------

Hi Ben,

I noticed some inconsistency of feature names with the ethtool getting/setting of features mechanics -- the name of the feature you need to set (through -K) isn't what displayed by the get (-k) directive, here's an example:

$ ethtool -k eth1  | grep generic-receive-offload
generic-receive-offload: on

$ ethtool -K eth1  generic-receive-offload off
ethtool: bad command line argument(s)
For more information run ethtool -h

--> looking in the sources and realizing I need to use "rx-gro"

$ ethtool -K eth1  rx-gro on

$ethtool -k eth1  | grep generic-receive-offload
generic-receive-offload: on

same problem for rx checksum which is displayed as "rx-checksumming" by the get (-k)
but need to be "rx-checksum" for the set (-K) directive.

Re: (repost for 2020y) inconsistency of ethtool feature names for get vs. set

[Michal Kubecek]

On Wed, Apr 08, 2020 at 04:07:39PM +0300, Konstantin Kharlamov wrote:
> I noticed some inconsistency of feature names with the ethtool getting/setting of features mechanics -- the name of the feature you need to set (through -K) isn't what displayed by the get (-k) directive, here's an example:
> 
> $ ethtool -k eth1  | grep generic-receive-offload
> generic-receive-offload: on
> 
> $ ethtool -K eth1  generic-receive-offload off
> ethtool: bad command line argument(s)
> For more information run ethtool -h
> 
> --> looking in the sources and realizing I need to use "rx-gro"
> 
> $ ethtool -K eth1  rx-gro on
> 
> $ethtool -k eth1  | grep generic-receive-offload
> generic-receive-offload: on
> 
> same problem for rx checksum which is displayed as "rx-checksumming" by the get (-k)
> but need to be "rx-checksum" for the set (-K) directive.

This is actually going to work with netlink implementation in kernel
5.7-rc1 (or current mainline) and corresponding userspace patchset I'm
going to submit after ethtool 5.6 is released:

  lion:~ # ~mike/work/git/ethtool/ethtool -k dummy0 | grep generic-receive-offload
  generic-receive-offload: on
  lion:~ # ~mike/work/git/ethtool/ethtool -K dummy0 generic-receive-offload off
  Actual changes:
  rx-gro: off
  lion:~ # ~mike/work/git/ethtool/ethtool -k dummy0 | grep generic-receive-offload
  generic-receive-offload: off

(The "rx-gro" in "Actual changes" section which probably comes from
using a dump function in ioctl code. I'll look into it.)

But independent of that, the ioctl code path should also accept actual
feature names provided by kernel. I'll try to find where the problem is
and fix it.

Michal Kubecek

Re: (repost for 2020y) inconsistency of ethtool feature names for get vs. set

[Michal Kubecek]

On Thu, Apr 09, 2020 at 09:40:14PM +0200, Michal Kubecek wrote:
> On Wed, Apr 08, 2020 at 04:07:39PM +0300, Konstantin Kharlamov wrote:
> > I noticed some inconsistency of feature names with the ethtool getting/setting of features mechanics -- the name of the feature you need to set (through -K) isn't what displayed by the get (-k) directive, here's an example:
> > 
> > $ ethtool -k eth1  | grep generic-receive-offload
> > generic-receive-offload: on
> > 
> > $ ethtool -K eth1  generic-receive-offload off
> > ethtool: bad command line argument(s)
> > For more information run ethtool -h
> > 
> > --> looking in the sources and realizing I need to use "rx-gro"
> > 
> > $ ethtool -K eth1  rx-gro on
> > 
> > $ethtool -k eth1  | grep generic-receive-offload
> > generic-receive-offload: on
> > 
> > same problem for rx checksum which is displayed as "rx-checksumming" by the get (-k)
> > but need to be "rx-checksum" for the set (-K) directive.
> 
> But independent of that, the ioctl code path should also accept actual
> feature names provided by kernel. I'll try to find where the problem is
> and fix it.

After reading the code, I have to correct this: the problem is the
opposite: kernel feature name is "rx-gro" and that does work. However,
this feature (NETIF_F_GRO) belongs under one of the legacy "flags" and
as it's the only one for that flag, ethtool does show flag name instead
of actual feature name (for backward compatibility).

The real problem is that each legacy flag has two names: short and long
(in this case, short is "gro" and long "generic-receive-offload"). And
while "ethtool -k" shows long flag names, "ethtool -K" only accepts
short ones (so that "ethtool -K eth1 gro off" would work).

I still agree that "ethtool -K" not accepting the names tha "ethtool -k"
is unfortunate and it should be fixed. A quick fix below seems to do the
trick but I'll have to run few more tests before submitting it.

A loosely related question is if NETIF_F_GRO_HW ("rx-gro-hw") and
NETIF_F_GRO_FRAGLIST_BIT ("rx-gro-list") shouldn't belong to this legacy
flag (gro / generic-receive-offload) as well. But that would require
also changes on kernel side and it's questionable if we really want to
further improve the concept of legacy flags when it would be cleaner to
get rid of them (which we cannot as it would break the backward
compatibility).

Michal Kubecek


diff --git a/ethtool.c b/ethtool.c
index 1b4e08b6e60f..27411ae776f4 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2297,24 +2297,31 @@ static int do_sfeatures(struct cmd_context *ctx)
 	/* Generate cmdline_info for legacy flags and kernel-named
 	 * features, and parse our arguments.
 	 */
-	cmdline_features = calloc(ARRAY_SIZE(off_flag_def) + defs->n_features,
+	cmdline_features = calloc(2 * ARRAY_SIZE(off_flag_def) +
+				  defs->n_features,
 				  sizeof(cmdline_features[0]));
 	if (!cmdline_features) {
 		perror("Cannot parse arguments");
 		rc = 1;
 		goto err;
 	}
-	for (i = 0; i < ARRAY_SIZE(off_flag_def); i++)
+	j = 0;
+	for (i = 0; i < ARRAY_SIZE(off_flag_def); i++) {
 		flag_to_cmdline_info(off_flag_def[i].short_name,
 				     off_flag_def[i].value,
 				     &off_flags_wanted, &off_flags_mask,
-				     &cmdline_features[i]);
+				     &cmdline_features[j++]);
+		flag_to_cmdline_info(off_flag_def[i].long_name,
+				     off_flag_def[i].value,
+				     &off_flags_wanted, &off_flags_mask,
+				     &cmdline_features[j++]);
+	}
 	for (i = 0; i < defs->n_features; i++)
 		flag_to_cmdline_info(
 			defs->def[i].name, FEATURE_FIELD_FLAG(i),
 			&FEATURE_WORD(efeatures->features, i, requested),
 			&FEATURE_WORD(efeatures->features, i, valid),
-			&cmdline_features[ARRAY_SIZE(off_flag_def) + i]);
+			&cmdline_features[j++]);
 	parse_generic_cmdline(ctx, &any_changed, cmdline_features,
 			      ARRAY_SIZE(off_flag_def) + defs->n_features);
 	free(cmdline_features);