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