Re: [RFC ethtool 6/6] netlink: add support for standard stats

On Sat, Apr 17, 2021 at 11:47:28AM -0700, Jakub Kicinski wrote:

On Sat, 17 Apr 2021 21:14:40 +0300 Ido Schimmel wrote:

quoted

quoted

+	if (!is_json_context()) {
+		fprintf(stdout, "rmon-%s-etherStatsPkts",
+			mnl_attr_get_type(hist) == ETHTOOL_A_STATS_GRP_HIST_RX ?
+			"rx" : "tx");
+
+		if (low && hi) {
+			fprintf(stdout, "%uto%uOctets: %llu\n", low, hi, val);
+		} else if (hi) {
+			fprintf(stdout, "%uOctets: %llu\n", hi, val);
+		} else if (low) {
+			fprintf(stdout, "%utoMaxOctets: %llu\n", low, val);
+		} else {
+			fprintf(stderr, "invalid kernel response - bad histogram entry bounds\n");
+			return 1;
+		}
+	} else {
+		open_json_object(NULL);
+		print_uint(PRINT_JSON, "low", NULL, low);
+		print_uint(PRINT_JSON, "high", NULL, hi);
+		print_u64(PRINT_JSON, "val", NULL, val);  

In the non-JSON output you distinguish between Rx/Tx, but it's missing
from the JSON output as can be seen in your example:

       "pktsNtoM": [
         {
           "low": 0,
           "high": 64,
           "val": 1
         },
         {
           "low": 128,
           "high": 255,
           "val": 1
         },
         {
           "low": 1024,
           "high": 0,
           "val": 0
         }
       ]

I see that mlxsw and mlx5 only support Rx, but it's going to be
confusing with bnxt that supports both Rx and Tx.

Good catch! I added Tx last minute (even though it's non standard).
I'll split split into two arrays - "rx-pktsNtoM" and "tx-pktsNtoM",
sounds good? Or we can add a layer: ["pktsNtoM"]["rx"] etc.

I'm fine with both, but I think the first form will be easier when
working with jq to extract Rx/Tx. It is also more inline with the
current nesting of the netlink attributes.

quoted

Made me think about the structure of these attributes. Currently you
have:

ETHTOOL_A_STATS_GRP_HIST_RX
	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
	ETHTOOL_A_STATS_GRP_HIST_VAL

ETHTOOL_A_STATS_GRP_HIST_TX
	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
	ETHTOOL_A_STATS_GRP_HIST_VAL

Did you consider:

ETHTOOL_A_STATS_GRP_HIST
	ETHTOOL_A_STATS_GRP_HIST_BKT_LOW
	ETHTOOL_A_STATS_GRP_HIST_BKT_HI
	ETHTOOL_A_STATS_GRP_HIST_VAL
	ETHTOOL_A_STATS_GRP_HIST_BKT_UNITS
	ETHTOOL_A_STATS_GRP_HIST_TYPE

I went back and forth on that. The reason I put the direction in the
type is that normal statistics don't have an extra _TYPE or direction.

It will also be easier to break the stats out to arrays if they are
typed on the outside, see below.

quoted

So you will have something like:

ETHTOOL_A_STATS_GRP_HIST_BKT_UNITS_BYTES

Histogram has two dimensions, what's the second dimension for bytes?
Time? Packet arrival?

Not sure what you mean. Here you are counting how many Rx/Tx packets are
between N to M bytes in length. I meant to add two attributes. One that
tells user space that you are counting Rx/Tx packets and the second that
N to M are in bytes.

But given your comment below about this histogram probably being a one
time thing, I think maybe staying with the current attributes is OK.
There is no need to over-engineer it if we don't see ourselves adding
new histograms.

Anyway, these histograms are under ETHTOOL_A_STATS_GRP that should give
user space all the context about what is being counted.

quoted

ETHTOOL_A_STATS_GRP_HIST_VAL_TYPE_RX_PACKETS
ETHTOOL_A_STATS_GRP_HIST_VAL_TYPE_TX_PACKETS

And it will allow you to get rid of the special casing of the RMON stuff
below:

	if (id == ETHTOOL_STATS_RMON) {
		open_json_array("pktsNtoM", "");

		mnl_attr_for_each_nested(attr, grp) {
			s = mnl_attr_get_type(attr);
			if (s != ETHTOOL_A_STATS_GRP_HIST_RX &&
			    s != ETHTOOL_A_STATS_GRP_HIST_TX)
				continue;

			if (parse_rmon_hist(attr))
				goto err_close_rmon;
		}
		close_json_array("");
	}

We can drop the if, but we still need a separate for() loop
to be able to place those entries in a JSON array.

quoted

I don't know how many histograms we are going to have as part of RFCs,
but at least mlxsw also supports histograms of the Tx queue depth and
latency. Not to be exposed by this interface, but shows the importance
of encoding the units.

TBH I hope we'll never use the hist for anything else. Sadly the
bucketing of various drivers is really different (at least 6
variants). But the overarching goal is a common interface for common
port stats.