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

["tj@kernel.org" <tj@kernel.org>]

Hello, Boy.

On Fri, Jul 19, 2024 at 01:47:35AM +0000, Boy Wu (吳勃誼) wrote:
...
> 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

Yeah, but it can't be used when there are multiple updaters.

> 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?

It doesn't make sense to protect u64_sync with a spinlock. Both are only
needed on 32bit. What's the point of having both? Also, note that iostat_cpu
is also updated from two paths - bio issue and stat reset. If you want to
keep that u64_sync, the only way to avoid possible deadlocks is adding
spinlock in the bio issue path too, which will be pretty expensive.

> > 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?

It's the same issue. Reset is another writer and whenever you have more than
one writers w/ u64_sync, there's a chance of deadlocks. So, iostat_cpu also
has two writers - bio issue and stat reset. If you want to keep using
u64_sync in both places, the only way to do it is adding spinlock protection
to both paths, which is an expensive thing to do for the bio issue path. So,
here, we'd rather just give up and let stat resetting be racy on 32bit.

Thanks.

-- 
tejun