[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241107200940.4dff026d@kernel.org>
Date: Thu, 7 Nov 2024 20:09:40 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Moon Yeounsu <yyyynoom@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org, andrew@...n.ch
Subject: Re: [PATCH net-next v2] net: dlink: add support for reporting stats
via `ethtool -S`
On Fri, 8 Nov 2024 00:19:33 +0900 Moon Yeounsu wrote:
> +}, mac_stats[] = {
> + DEFINE_MAC_STATS(tx_packets, FramesXmtOk,
> + u32, FramesTransmittedOK),
> + DEFINE_MAC_STATS(rx_packets, FramesRcvOk,
> + u32, FramesReceivedOK),
> + DEFINE_MAC_STATS(tx_bytes, OctetXmtOk,
> + u32, OctetsTransmittedOK),
> + DEFINE_MAC_STATS(rx_bytes, OctetRcvOk,
> + u32, OctetsReceivedOK),
> + DEFINE_MAC_STATS(single_collisions, SingleColFrames,
> + u32, SingleCollisionFrames),
> + DEFINE_MAC_STATS(multi_collisions, MultiColFrames,
> + u32, MultipleCollisionFrames),
> + DEFINE_MAC_STATS(late_collisions, LateCollisions,
> + u32, LateCollisions),
> + DEFINE_MAC_STATS(rx_frames_too_long_errors, FrameTooLongErrors,
> + u16, FrameTooLongErrors),
> + DEFINE_MAC_STATS(rx_in_range_length_errors, InRangeLengthErrors,
> + u16, InRangeLengthErrors),
> + DEFINE_MAC_STATS(rx_frames_check_seq_errors, FramesCheckSeqErrors,
> + u16, FrameCheckSequenceErrors),
> + DEFINE_MAC_STATS(rx_frames_lost_errors, FramesLostRxErrors,
> + u16, FramesLostDueToIntMACRcvError),
> + DEFINE_MAC_STATS(tx_frames_abort, FramesAbortXSColls,
> + u16, FramesAbortedDueToXSColls),
> + DEFINE_MAC_STATS(tx_carrier_sense_errors, CarrierSenseErrors,
> + u16, CarrierSenseErrors),
> + DEFINE_MAC_STATS(tx_multicast_frames, McstFramesXmtdOk,
> + u32, MulticastFramesXmittedOK),
> + DEFINE_MAC_STATS(rx_multicast_frames, McstFramesRcvdOk,
> + u32, MulticastFramesReceivedOK),
> + DEFINE_MAC_STATS(tx_broadcast_frames, BcstFramesXmtdOk,
> + u16, BroadcastFramesXmittedOK),
> + DEFINE_MAC_STATS(rx_broadcast_frames, BcstFramesRcvdOk,
> + u16, BroadcastFramesReceivedOK),
> + DEFINE_MAC_STATS(tx_frames_deferred, FramesWDeferredXmt,
> + u32, FramesWithDeferredXmissions),
> + DEFINE_MAC_STATS(tx_frames_excessive_deferral, FramesWEXDeferal,
> + u16, FramesWithExcessiveDeferral),
> +}, rmon_stats[] = {
> + DEFINE_RMON_STATS(rmon_under_size_packets,
> + EtherStatsUndersizePkts, undersize_pkts),
> + DEFINE_RMON_STATS(rmon_fragments,
> + EtherStatsFragments, fragments),
> + DEFINE_RMON_STATS(rmon_jabbers,
> + EtherStatsJabbers, jabbers),
> + DEFINE_RMON_STATS(rmon_tx_byte_64,
> + EtherStatsPkts64OctetTransmit, hist_tx[0]),
> + DEFINE_RMON_STATS(rmon_rx_byte_64,
> + EtherStats64Octets, hist[0]),
> + DEFINE_RMON_STATS(rmon_tx_byte_65to127,
> + EtherStats65to127OctetsTransmit, hist_tx[1]),
> + DEFINE_RMON_STATS(rmon_rx_byte_64to127,
> + EtherStatsPkts65to127Octets, hist[1]),
> + DEFINE_RMON_STATS(rmon_tx_byte_128to255,
> + EtherStatsPkts128to255OctetsTransmit, hist_tx[2]),
> + DEFINE_RMON_STATS(rmon_rx_byte_128to255,
> + EtherStatsPkts128to255Octets, hist[2]),
> + DEFINE_RMON_STATS(rmon_tx_byte_256to511,
> + EtherStatsPkts256to511OctetsTransmit, hist_tx[3]),
> + DEFINE_RMON_STATS(rmon_rx_byte_256to511,
> + EtherStatsPkts256to511Octets, hist[3]),
> + DEFINE_RMON_STATS(rmon_tx_byte_512to1023,
> + EtherStatsPkts512to1023OctetsTransmit, hist_tx[4]),
> + DEFINE_RMON_STATS(rmon_rx_byte_512to1203,
> + EtherStatsPkts512to1023Octets, hist[4]),
> + DEFINE_RMON_STATS(rmon_tx_byte_1204to1518,
> + EtherStatsPkts1024to1518OctetsTransmit, hist_tx[5]),
> + DEFINE_RMON_STATS(rmon_rx_byte_1204to1518,
> + EtherStatsPkts1024to1518Octets, hist[5])
> +};
Do these macro wrappers really buy you anything?
They make the code a lot harder to follow :(
> +static void get_ethtool_mac_stats(struct net_device *dev,
> + struct ethtool_eth_mac_stats *mac_base)
> +{
> + struct netdev_private *np = netdev_priv(dev);
> +
> + get_stats(dev);
> +
> + if (mac_base->src != ETHTOOL_MAC_STATS_SRC_AGGREGATE)
> + return;
> +
> + for (int i = 0; i < MAC_STATS_SIZE; i++)
> + READ_DATA(mac_stats, mac_base, i) = READ_STAT(mac_stats, np, i);
> +}
> +
> +
nit: multiple empty lines, checkpatch --strict should catch this
> +static void get_strings(struct net_device *dev, u32 stringset, u8 *data)
> +{
> + switch (stringset) {
> + case ETH_SS_STATS:
> + for (int i = 0; i < STATS_SIZE; i++) {
> + memcpy(data, stats[i].string, ETH_GSTRING_LEN);
> + data += ETH_GSTRING_LEN;
We've been getting conversion patches replacing such code with
ethtool_puts() lately, let's use it from the start.
--
pw-bot: cr
Powered by blists - more mailing lists