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
