ovl: ovl_fs::creator_cred::usage scalability issues

  *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. 
   OR
   2) In ovl_permission(), copy global creator_cred to a local variable to
      avoid concurrency.

With any mitigations above, CPU will not stall on overlayfs.

Paste mitigation 1 below.

From 472bd18eaabcde0d41e450f556691151b1bdb64e Mon Sep 17 00:00:00 2001
From: Chen Hu <hu1.chen@intel.com>
Date: Fri, 1 Sep 2023 15:03:28 +0800
Subject: [RFC PATCH] ovl: check underlying upper inode once in its lifetime

ovl_permission() checks the underlying inode with the credential of
mounter. The cred, struct ovl_fs::creator_cred, is global per overlayfs
superblock. Performance degrades when concurrency increases on the cred,
to be specific, on ovl_fs::creator_cred::usage.

This patch (or hack to some extent) checks underlying upper inode once
in its lifetime, eliminates the cache line contention on
creator_cred::usage and gets 40%+ perf improvement on a 128 cores CPU.

CAUTION:
this may compromise the file permission check. Need to talk with
overlayfs experts.

Signed-off-by: Chen Hu <hu1.chen@intel.com>
---
 fs/overlayfs/inode.c     | 5 ++++-
 fs/overlayfs/overlayfs.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)
  

On Wed, Oct 18, 2023 at 10:47 AM Chen Hu <hu1.chen@intel.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.