Message-ID: <c9e208e4-af14-424b-b09b-3e5068be2706@ti.com>
Date: Fri, 14 Mar 2025 12:15:34 +0530
From: MD Danish Anwar <danishanwar@...com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Vignesh Raghavendra <vigneshr@...com>, Meghana Malladi <m-malladi@...com>,
        Diogo Ivo <diogo.ivo@...mens.com>, Lee Trager <lee@...ger.us>,
        Andrew Lunn
	<andrew+netdev@...n.ch>,
        Roger Quadros <rogerq@...nel.org>, Jonathan Corbet
	<corbet@....net>,
        Simon Horman <horms@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller"
	<davem@...emloft.net>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <netdev@...r.kernel.org>, <srk@...com>
Subject: Re: [PATCH net-next v2] net: ti: icssg-prueth: Add ICSSG FW Stats



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

Powered by blists - more mailing lists