On Fri, 7 Mar 2025 16:00:40 +0530 MD Danish Anwar wrote: > > Thanks for the docs, it looks good. Now, do all of these get included > > in the standard stats returned by icssg_ndo_get_stats64 ? > > That's the primary source of information for the user regarding packet > > loss. > > No, these are not reported via icssg_ndo_get_stats64. > > .ndo_get_stats64 populates stats that are part of `struct > rtnl_link_stats64`. icssg_ndo_get_stats64 populates those stats wherever > applicable. These firmware stats are not same as the ones defined in > `icssg_ndo_get_stats64` hence they are not populated. They are not > standard stats, they will be dumped by `ethtool -S`. Wherever there is a > standard stats, I will make sure it gets dumped from the standard > interface instead of `ethtool -S` > > Only below stats are included in the standard stats returned by > icssg_ndo_get_stats64 > - rx_packets > - rx_bytes > - tx_packets > - tx_bytes > - rx_crc_errors > - rx_over_errors > - rx_multicast_frames Yes, but if the stats you're adding here relate to packets sent / destined to the host which were lost you should include them in the aggregate rx_errors / rx_dropped / tx_errors / tx_dropped. I understand that there's unlikely to be a 1:1 match with specific stats. > > This gets called by icssg_ndo_get_stats64() which is under RCU > > Yes, this does get called by icssg_ndo_get_stats64(). Apart from that > there is a workqueue (`icssg_stats_work_handler`) that calls this API > periodically and updates the emac->stats and emac->pa_stats arrays. > > > protection and nothing else. I don't see any locking here, and > > There is no locking here. I don't think this is related to the patch. > The API emac_update_hardware_stats() updates all the stats supported by > ICSSG not just standard stats. Yes, I'm saying you should send a separate fix, not really related or blocking this patch (unless they conflict) > > I hope the regmap doesn't sleep. cat /proc/net/dev to test. > > You probably need to send some fixes to net. > > I checked cat /proc/net/dev and the stats shown there are not related to > the stats I am introducing in this series. You misunderstood. I pointed that you so you can check on a debug kernel if there are any warnings (e.g. something tries to sleep since /proc/net/dev read is under RCU lock). > The fix could be to add a lock in this function, but we have close to 90 > stats and this function is called not only from icssg_ndo_get_stats64() > but from emac_get_ethtool_stats(). The function also gets called > periodically (Every 25 Seconds for 1G Link). I think every time locking > 90 regmap_reads() could result in performance degradation. Correctness comes first.
