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: <5a1e1bd5168696a0857b62572336c5e598cb5493.camel@mediatek.com>
Date: Mon, 15 Jul 2024 07:15:24 +0000
From: Boy Wu (吳勃誼) <Boy.Wu@...iatek.com>
To: "tj@...nel.org" <tj@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"boris@....io" <boris@....io>, "linux-block@...r.kernel.org"
	<linux-block@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "cgroups@...r.kernel.org"
	<cgroups@...r.kernel.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 Fri, 2024-07-12 at 08:38 -1000, tj@...nel.org wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hello, Boy.
> 
> On Fri, Jul 12, 2024 at 01:39:51AM +0000, Boy Wu (吳勃誼) wrote:
> ...
> > 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.
> 
> I don't get it. The only reader of blkg->iostat is
> blkcg_print_one_stat().
> It can just grab the same spin lock that the updaters use, right? Why
> do we
> also need u64_sync for blkg->iostat?
> 
> Thanks.
> 
> -- 
> tejun

I think I get your idea. You want to replace all the u64 sync for
iostat. However, I have one question: why use blkg_stat_lock instead of
adding a spin lock for each iostat like iostat.spinlock? We don't need
to lock between updating different iostats, but only lock when updating
the same iostat.

--
Boy.Wu