[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAOQ4uxg-WvdcuCrQg7zp03ocNZoT-G2bpi=Y6nVxMTodyFAUbg@mail.gmail.com>
Date: Wed, 18 Oct 2023 14:59:23 +0300
From: Amir Goldstein <amir73il@...il.com>
To: Chen Hu <hu1.chen@...el.com>
Cc: miklos@...redi.hu, malini.bhandaru@...el.com, tim.c.chen@...el.com,
mikko.ylinen@...el.com, lizhen.you@...el.com,
vinicius.gomes@...el.com, linux-unionfs@...r.kernel.org,
linux-kernel@...r.kernel.org,
Christian Brauner <brauner@...nel.org>
Subject: Re: ovl: ovl_fs::creator_cred::usage scalability issues
On Wed, Oct 18, 2023 at 10:47 AM Chen Hu <hu1.chen@...el.com> wrote:
>
> *Problem*
> ovl_permission() checks the underlying inode with the credential of mounter.
> The cred, struct ovl_fs::creator_cred, is somewhat global per overlayfs
> superblock. Performance degrades when concurrency increases on the cred, to be
> specific, on ovl_fs::creator_cred::usage.
>
> This happens when there are massive file access inside container, especially
> on SoC with many cores. With Linux 6.6.0-rc2, we run a web workload container
> on Intel 4th Xeon Sapphire Rapids which has 56 cores. Perf reports that 5.7%
> (2.50% + 1.87% + 1.33%) CPU stall in overlayfs:
> Self Command Shared Object Symbol
> 2.50% foo [kernel.vmlinux] [k] override_creds
> 1.87% foo [kernel.vmlinux] [k] revert_creds
> 1.33% foo [kernel.vmlinux] [k] generic_permission
>
> On Soc with more than 100 cores, we can even observe ~30% CPU stalled!
>
> This scalability issue is caused by two factors:
> 1) Contention on creator_cred::usage
> creator_cred::usage is atomic_t and is inc/dec atomically during every file
> access. So HW acquires the corresponding cache line exclusively. This
> operataiton is expensive and gets worse when contention is heavy.
> Call chain:
> ovl_permission()
> -> ovl_override_creds()
> -> override_creds()
> -> get_new_cred()
> -> atomic_inc(&cred->usage);
>
> ovl_permission()
> -> revert_creds()
> -> put_cred()
> -> atomic_dec_and_test(&(cred)->usage))
>
> 2) False sharing
> `perf c2c` shows false sharing issue between cred::usage and cred::fsuid.
> This is why generic_permission() stalls 1.33% CPU in above perf report.
> ovl_permission() updates cred::usage and it also reads cred::fsuid.
> Unfortunately, they locate in the same cache line and thus false sharing
> occurs. cred::fsuid is read at:
> ovl_permission()
> -> inode_permission()
> -> generic_permission()
> -> acl_permission_check()
> -> current_fsuid()
>
> *Mitigations we tried*
> We tried several mitigations but are not sure if it can be a fix or just
> workaround / hack. So we report this and want to have some discussions.
>
> Our mitigations aims to eliminate the contention on creator_cred->usage.
> Without contention, the false sharing will be tiny and no need to handle. The
> mitigations we tested are:
> 1) Check underlying inode once in its lifetime.
But the check is against a specific permission mask.
Your patch caches the result of the permission check of a specific mask
and uses it as the result to return for any mask.
> OR
> 2) In ovl_permission(), copy global creator_cred to a local variable to
> avoid concurrency.
>
This sounds a bit risky, but maybe it can work.
If you want to create a local copy of creds, I think that the fact that this
is a local copy should be expressed in flags like cred->non_rcu.
put_cred() should be aware of this flag and avoid calling __put_cred().
The local copy should be initialized with usage 1 by copying creator_cred
and we need to have an assertion if cred->usage drops to 0.
Also, ofs->creator_cred itself should be marked as a "read-only copy"
of credentials and we should add assertions to make sure that no code
calls get_cred() on a read-only copy.
The ovl_override_cred() function should take a local cred variable to use
the copy method for any access to ofs->creator_cred, not only in
ovl_permissions().
ovl_override_creds() should be coupled with ovl_revert_creds() which
also takes the local var as argument and also asserts that the local copy
usage is 1.
We can maybe take the opportunity to DEFINE_GUARD() for an
ovl_cred struct and use it in many of the overlayfs methods.
And maybe I am missing something and this cannot be done
or there is a much easier way to solve the problem.
Thanks,
Amir.
Powered by blists - more mailing lists