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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210417114728.660490a4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Sat, 17 Apr 2021 11:47:28 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     netdev@...r.kernel.org, idosch@...dia.com, mkubecek@...e.cz
Subject: Re: [RFC ethtool 6/6] netlink: add support for standard stats

On Sat, 17 Apr 2021 21:14:40 +0300 Ido Schimmel wrote:
> > +	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.

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

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

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

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