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: <70c37035f006de3a4fde895ceeee92826cd280e0.camel@mediatek.com>
Date: Fri, 12 Jul 2024 01:39:51 +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 Thu, 2024-07-11 at 11:02 -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,
> 
> 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