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:
> > > + 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.
>
> > 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?
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.
>
> > 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.
