Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for stats update

On Thu, 2024-07-11 at 11:02 -1000, tj@kernel.org wrote:

 	 
External email : Please do not click links or open attachments until
you have verified the sender or the content.
 Hello,

On Thu, Jul 11, 2024 at 02:25:29AM +0000, Boy Wu (吳勃誼) wrote:
...

quoted

I can remove the 32bit only define, but I think we need to add back

the

quoted

u64 sync, because the spin lock and the u64 sync serve different
purposes. The spin lock is for handling concurrent problems from
different CPUs updating stats, and u64 sync is for updating 64 bits
data and fetching 64 bits data from different CPUs in 32bit SMP
systems.

Hmm... so what I'm suggesting is using u64_sync for the per-cpu stat
structure as they are guaranteed to have only one updater with irq
disabled
and use a spinlock for the shared iostat which can have multiple
updaters
and isn't accessed that frequently.

Thanks.

-- 
tejun

I agree, but for multiple updaters, we not only need a spin lock but
also need u64_sync for 32bit SMP systems because u64_stats_fetch is not
protected by the spin lock blkg_stat_lock. If removing u64 sync, then
one CPU fetches data while another CPU is updating, may get a 64 bits
data with only 32 bits updated, while the other 32 bits are not updated
yet. We can see that blkcg_iostats_update is protected by both u64_sync
and the spin lock blkg_stat_lock in __blkcg_rstat_flush.
Thus, I think we should keep the u64_sync and just add the spin
lock blkg_stat_lock, not replace u64_sync with the spin lock.

--
Boy.Wu