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

[Boy Wu (吳勃誼)]

On Thu, 2024-07-11 at 11:02 -1000, tj@xxxxxxxxxx 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:
> ...
> > I can remove the 32bit only define, but I think we need to add back
> the
> > 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