On 6/2/23 11:58 PM, Jakub Kicinski wrote:
On Thu, 1 Jun 2023 15:12:28 -0700 Justin Chen wrote:+ /* general stats */ + STAT_NETDEV(rx_packets), + STAT_NETDEV(tx_packets), + STAT_NETDEV(rx_bytes), + STAT_NETDEV(tx_bytes), + STAT_NETDEV(rx_errors), + STAT_NETDEV(tx_errors), + STAT_NETDEV(rx_dropped), + STAT_NETDEV(tx_dropped), + STAT_NETDEV(multicast),please don't report standard interface stats in ethtool -S
These are not netdev statistics but MAC block counters. Guess it is not clear with the naming here, will fix this. We have a use case where the MAC traffic may be redirected from the associated net dev, so the counters may not be the same.
+ /* UniMAC RSV counters */ + STAT_BCMASP_MIB_RX("rx_64_octets", mib.rx.pkt_cnt.cnt_64), + STAT_BCMASP_MIB_RX("rx_65_127_oct", mib.rx.pkt_cnt.cnt_127), + STAT_BCMASP_MIB_RX("rx_128_255_oct", mib.rx.pkt_cnt.cnt_255), + STAT_BCMASP_MIB_RX("rx_256_511_oct", mib.rx.pkt_cnt.cnt_511), + STAT_BCMASP_MIB_RX("rx_512_1023_oct", mib.rx.pkt_cnt.cnt_1023), + STAT_BCMASP_MIB_RX("rx_1024_1518_oct", mib.rx.pkt_cnt.cnt_1518), + STAT_BCMASP_MIB_RX("rx_vlan_1519_1522_oct", mib.rx.pkt_cnt.cnt_mgv), + STAT_BCMASP_MIB_RX("rx_1522_2047_oct", mib.rx.pkt_cnt.cnt_2047), + STAT_BCMASP_MIB_RX("rx_2048_4095_oct", mib.rx.pkt_cnt.cnt_4095), + STAT_BCMASP_MIB_RX("rx_4096_9216_oct", mib.rx.pkt_cnt.cnt_9216),these should also be removed, and you should implement @get_rmon_stats.+ STAT_BCMASP_MIB_RX("rx_pkts", mib.rx.pkt), + STAT_BCMASP_MIB_RX("rx_bytes", mib.rx.bytes), + STAT_BCMASP_MIB_RX("rx_multicast", mib.rx.mca), + STAT_BCMASP_MIB_RX("rx_broadcast", mib.rx.bca), + STAT_BCMASP_MIB_RX("rx_fcs", mib.rx.fcs),there's a FCS error statistic in the standard stats, no need to duplicate
Same comment as above
+ STAT_BCMASP_MIB_RX("rx_control", mib.rx.cf), + STAT_BCMASP_MIB_RX("rx_pause", mib.rx.pf),@get_pause_stats+ STAT_BCMASP_MIB_RX("rx_unknown", mib.rx.uo), + STAT_BCMASP_MIB_RX("rx_align", mib.rx.aln), + STAT_BCMASP_MIB_RX("rx_outrange", mib.rx.flr), + STAT_BCMASP_MIB_RX("rx_code", mib.rx.cde), + STAT_BCMASP_MIB_RX("rx_carrier", mib.rx.fcr), + STAT_BCMASP_MIB_RX("rx_oversize", mib.rx.ovr), + STAT_BCMASP_MIB_RX("rx_jabber", mib.rx.jbr),these look like candidates from standard stats, too. Please read thru: https://docs.kernel.org/next/networking/statistics.html
Looks like the way we are doing stats are a bit outdated. Thanks for pointing it out. I got a bit of refactoring to do.
+ STAT_BCMASP_MIB_RX("rx_mtu_err", mib.rx.mtue), + STAT_BCMASP_MIB_RX("rx_good_pkts", mib.rx.pok), + STAT_BCMASP_MIB_RX("rx_unicast", mib.rx.uc), + STAT_BCMASP_MIB_RX("rx_ppp", mib.rx.ppp), + STAT_BCMASP_MIB_RX("rx_crc", mib.rx.rcrc),hm, what's the difference between rx_crc and rx_fcs ?
This looks like some debug feature that really has nothing to do with verifying crcs. I will remove it.
Apologies, probably should have done my due diligence with each stats instead of blindly including everything.
Thanks, Justin
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
