linux-kernel - Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat

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: <6114fbad7bce9b15806bb9fee25a1075ecb53dd1.camel@mediatek.com>
Date: Thu, 11 Jul 2024 02:25:29 +0000
From: Boy Wu (吳勃誼) <Boy.Wu@...iatek.com>
To: "tj@...nel.org" <tj@...nel.org>
CC: "boris@....io" <boris@....io>, "linux-block@...r.kernel.org"
	<linux-block@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "cgroups@...r.kernel.org"
	<cgroups@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "axboe@...nel.dk" <axboe@...nel.dk>,
	Iverlin Wang (王苳霖) <Iverlin.Wang@...iatek.com>,
	"josef@...icpanda.com" <josef@...icpanda.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v2] blk-cgroup: Replace u64_sync with blkg_stat_lock for
 stats update

On Wed, 2024-07-10 at 12:12 -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 Wed, Jul 10, 2024 at 02:13:34PM +0800, boy.wu wrote:
> ...
> > @@ -952,7 +952,6 @@ static void blkcg_fill_root_iostats(void)
> >  struct blkcg_gq *blkg = bdev->bd_disk->queue->root_blkg;
> >  struct blkg_iostat tmp;
> >  int cpu;
> > -unsigned long flags;
> >  
> >  memset(&tmp, 0, sizeof(tmp));
> >  for_each_possible_cpu(cpu) {
> > @@ -974,9 +973,10 @@ static void blkcg_fill_root_iostats(void)
> >  cpu_dkstats->sectors[STAT_DISCARD] << 9;
> >  }
> >  
> > -flags = u64_stats_update_begin_irqsave(&blkg->iostat.sync);
> > +#if BITS_PER_LONG == 32
> > +guard(raw_spinlock_irqsave)(&blkg_stat_lock);
> > +#endif
> >  blkg_iostat_set(&blkg->iostat.cur, &tmp);
> > -u64_stats_update_end_irqrestore(&blkg->iostat.sync, flags);
> 
> Isn't the problem shared across other blkg->iostat.sync users too? 

There are three places for iostat update sync.
1. The sync in blkcg_iostats_update is already protected by
blkg_stat_lock.
2. The sync in blkcg_fill_root_iostats is where we added it in the
patch.
3. The sync in blk_cgroup_bio_start is per CPU, and I don't think it
causes a concurrent problem.

> Also,
> maybe, we can just grab the spinlock without testing for 32bit. blkg-
> >iostat
> (unlike the per-cpu counterpart) isn't accessed that frequently, so
> keeping
> it simple and consistent probably makes more sense, right?

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.

> 
> Thanks.
> 
> -- 
> tejun

--
boy.wu