Re: [RFC net-next 4/6] ethtool: add interface to read standard MAC stats

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 14 Apr 2021 23:12:52 -0700 Saeed Mahameed wrote:
> On Wed, 2021-04-14 at 13:23 -0700, Jakub Kicinski wrote:
> > Most of the MAC statistics are included in
> > struct rtnl_link_stats64, but some fields
> > are aggregated. Besides it's good to expose
> > these clearly hardware stats separately.
> > 
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>

> > +/* Basic IEEE 802.3 MAC statistics (30.3.1.1.*), not otherwise exposed
> > + * via a more targeted API.
> > + */
> > +struct ethtool_eth_mac_stats {
> > +       u64 FramesTransmittedOK;
> > +       u64 SingleCollisionFrames;
> > +       u64 MultipleCollisionFrames;
> > +       u64 FramesReceivedOK;
> > +       u64 FrameCheckSequenceErrors;
> > +       u64 AlignmentErrors;
> > +       u64 OctetsTransmittedOK;
> > +       u64 FramesWithDeferredXmissions;
> > +       u64 LateCollisions;
> > +       u64 FramesAbortedDueToXSColls;
> > +       u64 FramesLostDueToIntMACXmitError;
> > +       u64 CarrierSenseErrors;
> > +       u64 OctetsReceivedOK;
> > +       u64 FramesLostDueToIntMACRcvError;
> > +       u64 MulticastFramesXmittedOK;
> > +       u64 BroadcastFramesXmittedOK;
> > +       u64 FramesWithExcessiveDeferral;
> > +       u64 MulticastFramesReceivedOK;
> > +       u64 BroadcastFramesReceivedOK;
> > +       u64 InRangeLengthErrors;
> > +       u64 OutOfRangeLengthField;
> > +       u64 FrameTooLongErrors;
> > +};
> > +
> >  /* Basic IEEE 802.3 PHY statistics (30.3.2.1.*), not otherwise exposed
> >   * via a more targeted API.
> >   */
> > @@ -495,6 +523,7 @@ struct ethtool_module_eeprom {
> >   *     specified page. Returns a negative error code or the amount of
> > bytes
> >   *     read.
> >   * @get_eth_phy_stats: Query some of the IEEE 802.3 PHY statistics.
> > + * @get_eth_mac_stats: Query some of the IEEE 802.3 MAC statistics.
> >   *
> >   * All operations are optional (i.e. the function pointer may be set
> >   * to %NULL) and callers must take this into account.  Callers must
> > @@ -607,6 +636,8 @@ struct ethtool_ops {
> >                                              struct netlink_ext_ack
> > *extack);
> >         void    (*get_eth_phy_stats)(struct net_device *dev,
> >                                      struct ethtool_eth_phy_stats
> > *phy_stats);
> > +       void    (*get_eth_mac_stats)(struct net_device *dev,
> > +                                    struct ethtool_eth_mac_stats
> > *mac_stats);  
> 
> too many callbacks.. I understand the point of having explicit structs
> per stats group, but it can be achievable with one generic ethtool
> calback function with the help of a flexible struct:
> 
> void (*get_std_stats)(struct net_device *dev, struct *std_stats)
> 
> 
> union stats_groups {
>     struct ethtool_eth_phy_stats eth_phy;
>     struct ethtool_eth_mac_stats eth_mac;
>     ...
> }
> 
> struct std_stats {
>      u16 type;
>      union stats_groups stats[0];
> }
> 
> where std_stats.stats is allocated dynamically according to
> std_stats.type
> 
> and driver can just access the corresponding stats according to type
> 
> e.g: 
> std_stats.stats.eth_phy

Kinda expected you'd say this :) The mux make life simpler for drivers
with a lot of layers of abstraction. Separate ops make life simpler for
simpler drivers.

Basic Ethernet driver goes from this:

get_mac_stats()
{
	priv = netdev_priv()

	stat->x = readl(priv->regs + REG_X);
	stat->z = readl(priv->regs + REG_Y);
	stat->y = readl(priv->regs + REG_Z);
}

to:

get_std_stats()
{
	priv = netdev_priv();

	switch (stats->type) {
	case MAC:
		stat->x = readl(priv->regs + REG_X);
		stat->z = readl(priv->regs + REG_Y);
		stat->y = readl(priv->regs + REG_Z);
		break;
	}
}

or likely:

get_std_stats()
{
	priv = netdev_priv();

	switch (stats->type) {
	case MAC:
		return get_mac_stats(priv..);
	}
}

I prefer to keep the callbacks separate, there isn't that many of them.

> > +static int stats_put_mac_stats(struct sk_buff *skb,
> > +                              const struct stats_reply_data *data)
> > +{
> > +       if (stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_2_TX_PKT,
> > +                    data->mac_stats.FramesTransmittedOK) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_3_SINGLE_COL,
> > +                    data->mac_stats.SingleCollisionFrames) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_4_MULTI_COL,
> > +                    data->mac_stats.MultipleCollisionFrames) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_5_RX_PKT,
> > +                    data->mac_stats.FramesReceivedOK) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_6_FCS_ERR,
> > +                    data->mac_stats.FrameCheckSequenceErrors) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_7_ALIGN_ERR,
> > +                    data->mac_stats.AlignmentErrors) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_8_TX_BYTES,
> > +                    data->mac_stats.OctetsTransmittedOK) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_9_TX_DEFER,
> > +                    data->mac_stats.FramesWithDeferredXmissions) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_10_LATE_COL,
> > +                    data->mac_stats.LateCollisions) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_11_XS_COL,
> > +                    data->mac_stats.FramesAbortedDueToXSColls) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_12_TX_INT_ERR,
> > +                    data->mac_stats.FramesLostDueToIntMACXmitError) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_13_CS_ERR,
> > +                    data->mac_stats.CarrierSenseErrors) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_14_RX_BYTES,
> > +                    data->mac_stats.OctetsReceivedOK) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_15_RX_INT_ERR,
> > +                    data->mac_stats.FramesLostDueToIntMACRcvError) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_18_TX_MCAST,
> > +                    data->mac_stats.MulticastFramesXmittedOK) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_19_TX_BCAST,
> > +                    data->mac_stats.BroadcastFramesXmittedOK) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_20_XS_DEFER,
> > +                    data->mac_stats.FramesWithExcessiveDeferral) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_21_RX_MCAST,
> > +                    data->mac_stats.MulticastFramesReceivedOK) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_22_RX_BCAST,
> > +                    data->mac_stats.BroadcastFramesReceivedOK) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_23_IR_LEN_ERR,
> > +                    data->mac_stats.InRangeLengthErrors) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_24_OOR_LEN,
> > +                    data->mac_stats.OutOfRangeLengthField) ||
> > +           stat_put(skb, ETHTOOL_A_STATS_ETH_MAC_25_TOO_LONG_ERR,
> > +                    data->mac_stats.FrameTooLongErrors))
> > +               return -EMSGSIZE;  
> 
> lots of repetition, someone might forget to add the new stat in one of
> these places .. 

If someone forgets to add a stat to the place they are dumped?
They will immediately realize it's not getting dumped...

> best practice here is to centralize all the data structures and
> information definitions in one place, you define the stat id, string,
> and value offset, then a generic loop can generate the strset and fill
> up values in the correct offset.
> 
> similar implementation is already in mlx5:
> 
> see pport_802_3_stats_desc:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c#L682
> 
> the "pport_802_3_stats_desc" has a description of the strings and
> offsets of all stats in this stats group
> and the fill/put functions are very simple and they just iterate over
> the array/group and fill up according to the descriptor.

We can maybe save 60 lines if we generate stats_eth_mac_names 
in a initcall, is it really worth it? I prefer the readability 
/ grepability.