Re: [PATCH v4] blk-cgroup: Replace u64 sync with spinlock for iostat

["Boy Wu (吳勃誼)" <Boy.Wu@mediatek.com>]

On Thu, 2024-07-18 at 11:15 -1000, Tejun Heo wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hello,
> 
> On Thu, Jul 18, 2024 at 04:41:12PM +0800, boy.wu wrote:
> >  static void blkg_clear_stat(struct blkcg_gq *blkg)
> >  {
> >  int cpu;
> >  
> > +#if BITS_PER_LONG == 32
> > +guard(raw_spinlock_irqsave)(&blkg_stat_lock);
> > +#endif
> 
> Can you please collect the ifdefs into a single place? If guard can
> be used
> for that, that's great. If not, just spin_lock/unlock wrappers are
> fine too,
> but please collect them into a single place and add a comment
> explaining why
> this is necessary and why u64_sync isn't being used.
> 
If it is for readability, I think keeping using u64 sync instead of
replacing it with spinlock is better, because what u64 sync protects is
64-bit data for 32-bit systems, while spinlock can be used for many
other reasons. The root cause of this issue is just the incorrect use
of u64 sync. Adding back the missing spinlock for the correct usage of
u64 sync is simpler. Is there any benefit to replacing u64 sync with
spinlock?

> Also, for blkg_clear_stat(), we're running a slight chance of
> clearing of
> iostat_cpu racing against state updates from the hot path. Given the
> circumstances - stat reset is an cgroup1-only feature which isn't
> used
> widely and a problematic interface to begin with, I believe this is a
> valid
> choice but it needs to be noted.

I don't get this part, but if this is another issue, maybe another
patch would be better?

> 
> Thanks.
> 
> -- 
> tejun

--
boy.wu