netdev - Re: [PATCH iwl-next v4 6/6] ice: convert all ring stats to u64_stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d2a14965-3ce6-466e-86ab-70c96a51992d@intel.com>
Date: Wed, 3 Dec 2025 14:21:04 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Simon Horman <horms@...nel.org>
CC: Aleksandr Loktionov <aleksandr.loktionov@...el.com>, Alexander Lobakin
	<aleksander.lobakin@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-next v4 6/6] ice: convert all ring stats to
 u64_stats_t



On 11/25/2025 2:17 AM, Simon Horman wrote:
> On Thu, Nov 20, 2025 at 12:20:46PM -0800, Jacob Keller wrote:
>> After several cleanups, the ice driver is now finally ready to convert all
>> Tx and Rx ring stats to the u64_stats_t and proper use of the u64 stats
>> APIs.
>>
>> The final remaining part to cleanup is the VSI stats accumulation logic in
>> ice_update_vsi_ring_stats().
>>
>> Refactor the function and its helpers so that all stat values (and not
>> just pkts and bytes) use the u64_stats APIs. The
>> ice_fetch_u64_(tx|rx)_stats functions read the stat values using
>> u64_stats_read and then copy them into local ice_vsi_(tx|rx)_stats
>> structures. This does require making a new struct with the stat fields as
>> u64.
>>
>> The ice_update_vsi_(tx|rx)_ring_stats functions call the fetch functions
>> per ring and accumulate the result into one copy of the struct. This
>> accumulated total is then used to update the relevant VSI fields.
>>
>> Since these are relatively small, the contents are all stored on the stack
>> rather than allocating and freeing memory.
>>
>> Once the accumulator side is updated, the helper ice_stats_read and
>> ice_stats_inc and other related helper functions all easily translate to
>> use of u64_stats_read and u64_stats_inc. This completes the refactor and
>> ensures that all stats accesses now make proper use of the API.
>>
>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> 
> Thanks.
> 
> I do notice in the cover that you solicit alternate approaches to
> lead to a yet cleaner solution. But I think that the approach you have
> taken does significantly improve both the cleanliness and correctness
> of the code. So even if we think of something better later, I think
> this is a good step to take now.
> 

Thanks. In particular, I was wondering if there is a way to help improve
or extend the overall u64_stats API to make some of this non-driver
specific.

It does seem like quite a lot of boilerplate code is needed to make
correct use of the APIs, especially with the u64_stat_t type existing
now which is necessary even on some 64-bit arch.

Perhaps some of the macro wrappers could migrate into u64_stats.h header
somehow. Though I'm not sure we can keep them quite as short without
being in the driver.

> Thanks for breaking out the series into bite-sized chunks, especially
> the last few patches. It really helped me in my review.
> 

Yep. I originally had this all munged together but it became impossible
to follow the logic of how I got to this point, and also obscured
several of the outright fixes.

> Reviewed-by: Simon Horman <horms@...nel.org>
> 



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)