From: Tejun Heo <tj@kernel.org>
To: "boy.wu" <boy.wu@mediatek.com>
Cc: Josef Bacik <josef@toxicpanda.com>, Jens Axboe <axboe@kernel.dk>,
Matthias Brugger <matthias.bgg@gmail.com>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com>,
Boris Burkov <boris@bur.io>,
cgroups@vger.kernel.org, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org, iverlin.wang@mediatek.com
Subject: Re: [PATCH v4] blk-cgroup: Replace u64 sync with spinlock for iostat
Date: Thu, 18 Jul 2024 11:15:28 -1000 [thread overview]
Message-ID: <ZpmF8HJsuefjC7Xr@slm.duckdns.org> (raw)
In-Reply-To: <20240718084112.12202-1-boy.wu@mediatek.com>
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.
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.
Thanks.
--
tejun
next prev parent reply other threads:[~2024-07-18 21:15 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-18 8:41 [PATCH v4] blk-cgroup: Replace u64 sync with spinlock for iostat boy.wu
2024-07-18 21:15 ` Tejun Heo [this message]
2024-07-19 1:47 ` Boy Wu (吳勃誼)
2024-07-19 17:31 ` tj
2024-07-26 3:43 ` Boy Wu (吳勃誼)
2024-07-30 19:49 ` tj
2024-08-09 7:50 ` Boy Wu (吳勃誼)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZpmF8HJsuefjC7Xr@slm.duckdns.org \
--to=tj@kernel.org \
--cc=angelogioacchino.delregno@collabora.com \
--cc=axboe@kernel.dk \
--cc=boris@bur.io \
--cc=boy.wu@mediatek.com \
--cc=cgroups@vger.kernel.org \
--cc=iverlin.wang@mediatek.com \
--cc=josef@toxicpanda.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox