Re: [RFC ethtool 6/6] netlink: add support for standard stats
On Sat, 17 Apr 2021 22:22:57 +0300 Ido Schimmel wrote: > > > 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. Ah, these were not part of the same enum, I get it now. I thought maybe bytes were trying to cater to the queue length use case and I wasn't sure how that histogram is constructed. > 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.
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:
> > > + 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. S
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.
Re: [RFC ethtool 6/6] netlink: add support for standard stats
On Fri, Apr 16, 2021 at 09:02:52AM -0700, Jakub Kicinski wrote:
> # ethtool -S eth0 --groups eth-phy eth-mac rmon
> Stats for eth0:
> eth-phy-SymbolErrorDuringCarrier: 1
> eth-mac-FramesTransmittedOK: 1
> eth-mac-FrameTooLongErrors: 1
> rmon-etherStatsUndersizePkts: 1
> rmon-etherStatsJabbers: 1
> rmon-etherStatsPkts64Octets: 1
> rmon-etherStatsPkts128to255Octets: 1
> rmon-etherStatsPkts1024toMaxOctets: 0
>
> # ethtool --json -S eth0 --groups eth-phy eth-mac rmon | jq
> [
> {
> "ifname": "eth0",
> "eth-phy": {
> "SymbolErrorDuringCarrier": 1
> },
> "eth-mac": {
> "FramesTransmittedOK": 1,
> "FrameTooLongErrors": 0
> },
> "rmon": {
> "etherStatsUndersizePkts": 1,
> "etherStatsJabbers": 0,
> "pktsNtoM": [
> {
> "low": 0,
> "high": 64,
> "val": 1
> },
> {
> "low": 128,
> "high": 255,
> "val": 1
> },
> {
> "low": 1024,
> "high": 0,
> "val": 0
> }
> ]
> }
> }
> ]
>
> # ethtool --json -S eth0 --groups eth-phy eth-mac rmon | \
> jq '.[].rmon.pktsNtoM | map(select(.low >= 128)) | map(.val) | add'
> 1
>
> Signed-off-by: Jakub Kicinski
> ---
> Makefile.am| 1 +
> ethtool.8.in | 11 ++
> ethtool.c | 5 +-
> netlink/desc-ethtool.c | 39 +++
> netlink/extapi.h | 4 +
> netlink/stats.c| 242 +
> 6 files changed, 301 insertions(+), 1 deletion(-)
> create mode 100644 netlink/stats.c
>
> diff --git a/Makefile.am b/Makefile.am
> index f643a24af97a..75c245653cda 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -36,6 +36,7 @@ ethtool_SOURCES += \
> netlink/features.c netlink/privflags.c netlink/rings.c \
> netlink/channels.c netlink/coalesce.c netlink/pause.c \
> netlink/eee.c netlink/tsinfo.c netlink/fec.c \
> + netlink/stats.c \
> netlink/desc-ethtool.c netlink/desc-genlctrl.c \
> netlink/desc-rtnl.c netlink/cable_test.c netlink/tunnels.c \
> uapi/linux/ethtool_netlink.h \
> diff --git a/ethtool.8.in b/ethtool.8.in
> index ba4e245cdc61..0ca11595158a 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -240,6 +240,12 @@ ethtool \- query or control network driver and hardware
> settings
> .HP
> .B ethtool \-S|\-\-statistics
> .I devname
> +.RB [\fB--groups
> +.RB [\fBeth\-phy\fP]
> +.RB [\fBeth\-mac\fP]
> +.RB [\fBeth\-ctrl\fP]
> +.RN [\fBrmon\fP]
> +.RB ]
> .HP
> .B ethtool \-\-phy\-statistics
> .I devname
> @@ -653,6 +659,11 @@ auto-negotiation is enabled.
> .B \-S \-\-statistics
> Queries the specified network device for NIC- and driver-specific
> statistics.
> +.RS 4
> +.TP
> +.B \fB--groups [\fBeth\-phy\fP] [\fBeth\-mac\fP] [\fBeth\-ctrl\fP]
> [\fBrmon\fP]
> +Request standard device statistics of a specific group.
> +.RE
> .TP
> .B \-\-phy\-statistics
> Queries the specified network device for PHY specific statistics.
> diff --git a/ethtool.c b/ethtool.c
> index b07fd9292d77..1b5690f424c8 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -5760,7 +5760,10 @@ static const struct option args[] = {
> {
> .opts = "-S|--statistics",
> .func = do_gnicstats,
> - .help = "Show adapter statistics"
> + .nlchk = nl_gstats_chk,
> + .nlfunc = nl_gstats,
> + .help = "Show adapter statistics",
> + .xhelp = " [ --groups [eth-phy] [eth-mac]
> [eth-ctrl] [rmon] ]\n"
> },
> {
> .opts = "--phy-statistics",
> diff --git a/netlink/desc-ethtool.c b/netlink/desc-ethtool.c
> index 9da052a5b8aa..63e91ee7ffcd 100644
> --- a/netlink/desc-ethtool.c
> +++ b/netlink/desc-ethtool.c
> @@ -325,6 +325,43 @@ static const struct pretty_nla_desc __fec_desc[] = {
> NLATTR_DESC_U32(ETHTOOL_A_FEC_ACTIVE),
> };
>
> +static const struct pretty_nla_desc __stats_grp_stat_desc[] = {
> + NLATTR_DESC_U64(0), NLATTR_DESC_U64(1), NLATTR_DESC_U64(2),
> + NLATTR_DESC_U64(3), NLATTR_DESC_U64(4), NLATTR_DESC_U64(5),
> + NLATTR_DESC_U64(6), NLATTR_DESC_U64(7), NLATTR_DESC_U64(8),
> + NLATTR_DESC_U64(9), NLATTR_DESC_U64(10), NLATTR_DESC_U64(11),
> + NLATTR_DESC_U64(12), NLATTR_DESC_U64(13), NLATTR_DESC_U64(14),
> + NLATTR_DESC_U64(15), NLATTR_DESC_U64(16), NLATTR_DESC_U64(17),
> + NLATTR_DESC_U64(18), NLATTR_DESC_U64(19), NLATTR_DESC_U64(20),
> + NLATTR_DESC_U64(21), NLATTR_DESC_U64(22), NLATTR_DESC_U64(23),
> + NLATTR_DESC_U64(24), NLATTR_DESC_U64(25), NLATTR_DESC_U64(26),
> + NLATTR_DESC_U64(27), NLATTR_DESC_U64(28), NLATTR_DESC_U64(29),
> +};
> +
> +static const struct pretty_nla_desc __stats_grp_hist_desc[] = {
> + NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_HIST_BKT_LOW),
> + NLATTR_DESC_U32(ETHTOOL_A_STATS_GRP_HIST_BKT_HI),
