On 07/03/25 10:09 pm, Jakub Kicinski wrote: > 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. > Sure, I will try to add such 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) > Sure. I will send v3 of this and a fix to net to add spin_lock before reading stats. I will try to make sure that they don't conflict so that they can be merged parallelly. >>> 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. Understood. -- Thanks and Regards, Danish
