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

[Saeed Mahameed <saeed@kernel.org>]

On Thu, 2021-04-15 at 08:38 -0700, Jakub Kicinski wrote:
> 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.
> 

Ack, i don't like switch cases, but i also don't like long structs with
pages and pages of callbacks..

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

kinda my point, I wouldn't count on this.. 

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

I don't think readability will be an issue if the infrastructure is
generic enough.. 

This just a preference, of course you can go with the current code.
My point is that someone doesn't need to change multiple places and
possibly files every time they need to expose a new stat, you just
update some central database of the new data you want to expose.