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.
