Fix file lock cache accounting, again

A container can exceed its memcg limits by allocating a bunch of file locks.

This bug was originally fixed by commit 0f12156dff28 ("memcg: enable accounting for file lock caches"), but was later reverted by commit 3754707bcc3e ("Revert "memcg: enable accounting for file lock caches"") due to performance issues.

Unfortunately those performance issues were never addressed and the bug has remained unfixed for over two years.

Fix it by default but allow users to disable it with a cmdline option (flock_accounting=off).

Signed-off-by Josh Poimboeuf <[email protected]>

---
 .../admin-guide/kernel-parameters.txt         | 17 +++++++++++
 fs/locks.c                                    | 30 +++++++++++++++++--
 2 files changed, 45 insertions(+), 2 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +17 -0

index 6ee0f9a5da70..91987b06bc52 100644
@@ -1527,6 +1527,23 @@
 			See Documentation/admin-guide/sysctl/net.rst for
 			fb_tunnels_only_for_init_ns
 
+	flock_accounting=
+			[KNL] Enable/disable accounting for kernel
+			memory allocations related to file locks.
+			Format: { on | off }
+			Default: on
+			on:	Enable kernel memory accounting for file
+				locks.  This prevents task groups from
+				exceeding their memcg allocation limits.
+				However, it may cause slowdowns in the
+				flock() system call.
+			off:	Disable kernel memory accounting for
+				file locks.  This may allow a rogue task
+				to DoS the system by forcing the kernel
+				to allocate memory beyond the task
+				group's memcg limits.  Not recommended
+				unless you have trusted user space.
+
 	floppy=		[HW]
 			See Documentation/admin-guide/blockdev/floppy.rst.
 

fs/locks.c +26 -0

index cc7c117ee192..235ac56c557d 100644
@@ -2905,15 +2905,41 @@ static int __init proc_locks_init(void)
 fs_initcall(proc_locks_init);
 #endif
 
+static bool flock_accounting __ro_after_init = true;
+
+static int __init flock_accounting_cmdline(char *str)
+{
+	if (!str)
+		return -EINVAL;
+
+	if (!strcmp(str, "off"))
+		flock_accounting = false;
+	else if (!strcmp(str, "on"))
+		flock_accounting = true;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+early_param("flock_accounting", flock_accounting_cmdline);
+
+#define FLOCK_ACCOUNTING_MSG "WARNING: File lock accounting is disabled, container-triggered host memory exhaustion possible!\n"
+
 static int __init filelock_init(void)
 {
 	int i;
+	slab_flags_t flags = SLAB_PANIC;
+
+	if (!flock_accounting)
+		pr_err(FLOCK_ACCOUNTING_MSG);
+	else
+		flags |= SLAB_ACCOUNT;
 

I'm really not a fan of tunables or different kconfig options, especially for something niche like this.

I also question whether this accounting will show up under any real- world workloads, and whether it was just wrong to revert those patches back in 2021.

File locking is an activity where we inherently expect to block. Ideally we don't if the lock is uncontended of course, but it's always a possibility.

The benchmark that prompted the regression basically just tries to create and release a bunch of file locks as quickly as possible. Legitimate applications that do a lot of very rapid locking like this benchmark are basically non-existent. Usually the pattern is:

acquire lock
do some (relatively slow) I/O
release lock

In that sort of scenario, is this memcg accounting more than just line noise? I wonder whether we should just bite the bullet and see whether there are any real workloads that suffer due to SLAB_ACCOUNT being enabled on these caches?

That's a good point. If the microbenchmark isn't likely to be even remotely realistic, maybe we should just revert the revert until if/when somebody shows a real world impact.

Linus, any objections to that?

Quoted 6 lines · Josh Poimboeuf

That's a good point. If the microbenchmark isn't likely to be even remotely realistic, maybe we should just revert the revert until if/when somebody shows a real world impact.

Linus, any objections to that?

We use SLAB_ACCOUNT for much more common allocations like queued signals, so I would tend to agree with Jeff that it's probably just some not very interesting microbenchmark that shows any file locking effects from SLAB_ALLOC, not any real use.

That said, those benchmarks do matter. It's very easy to say "not relevant in the big picture" and then the end result is that everything is a bit of a pig.

And the regression was absolutely *ENORMOUS*. We're not talking "a few percent". We're talking a 33% regression that caused the revert:

   https://lore.kernel.org/lkml/20210907150757.GE17617@xsang-OptiPlex-9020/

I wish our SLAB_ACCOUNT wasn't such a pig. Rather than account every single allocation, it would be much nicer to account at a bigger granularity, possibly by having per-thread counters first before falling back to the obj_cgroup_charge. Whatever.

It's kind of stupid to have a benchmark that just allocates and deallocates a file lock in quick succession spend lots of time incrementing and decrementing cgroup charges for that repeated alloc/free.

However, that problem with SLAB_ACCOUNT is not the fault of file locking, but more of a slab issue.

End result: I think we should bring in Vlastimil and whoever else is doing SLAB_ACCOUNT things, and have them look at that side.

And then just enable SLAB_ACCOUNT for file locks. But very much look at silly costs in SLAB_ACCOUNT first, at least for trivial "alloc/free" patterns..

Vlastimil? Who would be the best person to look at that SLAB_ACCOUNT thing? See commit 3754707bcc3e (Revert "memcg: enable accounting for file lock caches") for the history here.

Quoted 43 lines · Linus Torvalds

Quoted 6 lines · Josh Poimboeuf

That's a good point. If the microbenchmark isn't likely to be even remotely realistic, maybe we should just revert the revert until if/when somebody shows a real world impact.

Linus, any objections to that?

We use SLAB_ACCOUNT for much more common allocations like queued signals, so I would tend to agree with Jeff that it's probably just some not very interesting microbenchmark that shows any file locking effects from SLAB_ALLOC, not any real use.

That said, those benchmarks do matter. It's very easy to say "not relevant in the big picture" and then the end result is that everything is a bit of a pig.

And the regression was absolutely *ENORMOUS*. We're not talking "a few percent". We're talking a 33% regression that caused the revert:

   https://lore.kernel.org/lkml/20210907150757.GE17617@xsang-OptiPlex-9020/

I wish our SLAB_ACCOUNT wasn't such a pig. Rather than account every single allocation, it would be much nicer to account at a bigger granularity, possibly by having per-thread counters first before falling back to the obj_cgroup_charge. Whatever.

It's kind of stupid to have a benchmark that just allocates and deallocates a file lock in quick succession spend lots of time incrementing and decrementing cgroup charges for that repeated alloc/free.

However, that problem with SLAB_ACCOUNT is not the fault of file locking, but more of a slab issue.

End result: I think we should bring in Vlastimil and whoever else is doing SLAB_ACCOUNT things, and have them look at that side.

And then just enable SLAB_ACCOUNT for file locks. But very much look at silly costs in SLAB_ACCOUNT first, at least for trivial "alloc/free" patterns..

Quoted 4 lines

Vlastimil? Who would be the best person to look at that SLAB_ACCOUNT thing? See commit 3754707bcc3e (Revert "memcg: enable accounting for file lock caches") for the history here.

Roman last looked into optimizing this code path. I suspect mod_objcg_state() to be more costly than obj_cgroup_charge(). I will try to measure this path and see if I can improve it.

Quoted 45 lines · Shakeel Butt

Quoted 43 lines · Linus Torvalds

Quoted 6 lines · Josh Poimboeuf

That's a good point. If the microbenchmark isn't likely to be even remotely realistic, maybe we should just revert the revert until if/when somebody shows a real world impact.

Linus, any objections to that?

We use SLAB_ACCOUNT for much more common allocations like queued signals, so I would tend to agree with Jeff that it's probably just some not very interesting microbenchmark that shows any file locking effects from SLAB_ALLOC, not any real use.

That said, those benchmarks do matter. It's very easy to say "not relevant in the big picture" and then the end result is that everything is a bit of a pig.

And the regression was absolutely *ENORMOUS*. We're not talking "a few percent". We're talking a 33% regression that caused the revert:

   https://lore.kernel.org/lkml/20210907150757.GE17617@xsang-OptiPlex-9020/

I wish our SLAB_ACCOUNT wasn't such a pig. Rather than account every single allocation, it would be much nicer to account at a bigger granularity, possibly by having per-thread counters first before falling back to the obj_cgroup_charge. Whatever.

It's kind of stupid to have a benchmark that just allocates and deallocates a file lock in quick succession spend lots of time incrementing and decrementing cgroup charges for that repeated alloc/free.

However, that problem with SLAB_ACCOUNT is not the fault of file locking, but more of a slab issue.

End result: I think we should bring in Vlastimil and whoever else is doing SLAB_ACCOUNT things, and have them look at that side.

And then just enable SLAB_ACCOUNT for file locks. But very much look at silly costs in SLAB_ACCOUNT first, at least for trivial "alloc/free" patterns..

Quoted 8 lines

Quoted 4 lines

Vlastimil? Who would be the best person to look at that SLAB_ACCOUNT thing? See commit 3754707bcc3e (Revert "memcg: enable accounting for file lock caches") for the history here.

Roman last looked into optimizing this code path. I suspect mod_objcg_state() to be more costly than obj_cgroup_charge(). I will try to measure this path and see if I can improve it.

It's roughly an equal split between mod_objcg_state() and obj_cgroup_charge(). And each is comparable (by order of magnitude) to the slab allocation cost itself. On the free() path a significant cost comes simple from reading the objcg pointer (it's usually a cache miss).

So I don't see how we can make it really cheap (say, less than 5% overhead) without caching pre-accounted objects.

I thought about merging of charge and stats handling paths, which _maybe_ can shave off another 20-30%, but there still will be a double-digit% accounting overhead.

I'm curious to hear other ideas and suggestions.

Thanks!

Quoted 3 lines

So I don't see how we can make it really cheap (say, less than 5% overhead) without caching pre-accounted objects.

Maybe this is what we want. Now we are down to just SLUB, maybe such caching of pre-accounted objects can be in SLUB layer and we can decide to keep this caching per-kmem-cache opt-in or always on.

Quoted 7 lines · Shakeel Butt

Quoted 3 lines

So I don't see how we can make it really cheap (say, less than 5% overhead) without caching pre-accounted objects.

Maybe this is what we want. Now we are down to just SLUB, maybe such caching of pre-accounted objects can be in SLUB layer and we can decide to keep this caching per-kmem-cache opt-in or always on.

So it turns out that we have another case of SLAB_ACCOUNT being quite a big expense, and it's actually the normal - but failed - open() or execve() case.

See the thread at

https://lore.kernel.org/all/CAHk-=whw936qzDLBQdUz-He5WK_0fRSWwKAjtbVsMGfX70Nf_Q@mail.gmail.com/

and to see the effect in profiles, you can use this EXTREMELY stupid test program:

    #include <fcntl.h>

    int main(int argc, char **argv)
    {
        for (int i = 0; i < 10000000; i++)
                open("nonexistent", O_RDONLY);
    }

where the point of course is that the "nonexistent" pathname doesn't actually exist (so don't create a file called that for the test).

What happens is that open() allocates a 'struct file *' early from the filp kmem_cache, which has SLAB_ACCOUNT set. So we'll do accounting for it, failt the pathname open, and free it again, which uncharges the accounting.

Now, in this case, I actually have a suggestion: could we please just make SLAB_ACCOUNT be something that we do *after* the allocation, kind of the same way the zeroing works?

IOW, I'd love to get rid of slab_pre_alloc_hook() entirely, and make slab_post_alloc_hook() do all the "charge the memcg if required".

Obviously that means that now a failure to charge the memcg would have to then de-allocate things, but that's an uncommon path and would be marked unlikely and not be in the hot path at all.

Now, the reason I would prefer that is that the *second* step would be to

(a) expose a "kmem_cache_charge()" function that takes a
*non*-accounted slab allocation, and turns it into an accounted one (and obviously this is why you want to do everything in the post-alloc
hook: just try to share this code)

(b) remote the SLAB_ACCOUNT from the filp_cachep, making all file
allocations start out unaccounted.

(c) when we have *actually* looked up the pathname and open the file
successfully, at *that* point we'd do a

error = kmem_cache_charge(filp_cachep, file);

in do_dentry_open() to turn the unaccounted file pointer into an
accounted one (and if that fails, we do the cleanup and free it, of course, exactly like we do when file_get_write_access() fails)

which means that now the failure case doesn't unnecessarily charge the allocation that never ends up being finalized.

NOTE! I think this would clean up mm/slub.c too, simply because it would get rid of that memcg_slab_pre_alloc_hook() entirely, and get rid of the need to carry the "struct obj_cgroup **objcgp" pointer along until the post-alloc hook: everything would be done post-alloc.

The actual kmem_cache_free() code already deals with "this slab hasn't been accounted" because it obviously has to deal with allocations that were done without __GFP_ACCOUNT anyway. So there's no change needed on the freeing path, it already has to handle this all gracefully.

I may be missing something, but it would seem to have very little downside, and fix a case that actually is visible right now.

Quoted 7 lines · Shakeel Butt

Quoted 3 lines

So I don't see how we can make it really cheap (say, less than 5% overhead) without caching pre-accounted objects.

Maybe this is what we want. Now we are down to just SLUB, maybe such caching of pre-accounted objects can be in SLUB layer and we can decide to keep this caching per-kmem-cache opt-in or always on.

So it turns out that we have another case of SLAB_ACCOUNT being quite a big expense, and it's actually the normal - but failed - open() or execve() case.

See the thread at

https://lore.kernel.org/all/CAHk-=whw936qzDLBQdUz-He5WK_0fRSWwKAjtbVsMGfX70Nf_Q@mail.gmail.com/

and to see the effect in profiles, you can use this EXTREMELY stupid test program:

    #include <fcntl.h>

    int main(int argc, char **argv)
    {
        for (int i = 0; i < 10000000; i++)
                open("nonexistent", O_RDONLY);
    }

where the point of course is that the "nonexistent" pathname doesn't actually exist (so don't create a file called that for the test).

What happens is that open() allocates a 'struct file *' early from the filp kmem_cache, which has SLAB_ACCOUNT set. So we'll do accounting for it, failt the pathname open, and free it again, which uncharges the accounting.

Now, in this case, I actually have a suggestion: could we please just make SLAB_ACCOUNT be something that we do *after* the allocation, kind of the same way the zeroing works?

IOW, I'd love to get rid of slab_pre_alloc_hook() entirely, and make slab_post_alloc_hook() do all the "charge the memcg if required".

Obviously that means that now a failure to charge the memcg would have to then de-allocate things, but that's an uncommon path and would be marked unlikely and not be in the hot path at all.

Now, the reason I would prefer that is that the *second* step would be to

(a) expose a "kmem_cache_charge()" function that takes a
*non*-accounted slab allocation, and turns it into an accounted one (and obviously this is why you want to do everything in the post-alloc
hook: just try to share this code)

(b) remote the SLAB_ACCOUNT from the filp_cachep, making all file
allocations start out unaccounted.

(c) when we have *actually* looked up the pathname and open the file
successfully, at *that* point we'd do a

error = kmem_cache_charge(filp_cachep, file);

in do_dentry_open() to turn the unaccounted file pointer into an
accounted one (and if that fails, we do the cleanup and free it, of course, exactly like we do when file_get_write_access() fails)

which means that now the failure case doesn't unnecessarily charge the allocation that never ends up being finalized.

NOTE! I think this would clean up mm/slub.c too, simply because it would get rid of that memcg_slab_pre_alloc_hook() entirely, and get rid of the need to carry the "struct obj_cgroup **objcgp" pointer along until the post-alloc hook: everything would be done post-alloc.

The actual kmem_cache_free() code already deals with "this slab hasn't been accounted" because it obviously has to deal with allocations that were done without __GFP_ACCOUNT anyway. So there's no change needed on the freeing path, it already has to handle this all gracefully.

I may be missing something, but it would seem to have very little downside, and fix a case that actually is visible right now.

Linus

Quoted 20 lines · Linus Torvalds

Quoted 7 lines · Shakeel Butt

Quoted 3 lines

So I don't see how we can make it really cheap (say, less than 5% overhead) without caching pre-accounted objects.

Maybe this is what we want. Now we are down to just SLUB, maybe such caching of pre-accounted objects can be in SLUB layer and we can decide to keep this caching per-kmem-cache opt-in or always on.

So it turns out that we have another case of SLAB_ACCOUNT being quite a big expense, and it's actually the normal - but failed - open() or execve() case.

See the thread at

https://lore.kernel.org/all/CAHk-=whw936qzDLBQdUz-He5WK_0fRSWwKAjtbVsMGfX70Nf_Q@mail.gmail.com/

and to see the effect in profiles, you can use this EXTREMELY stupid test program:

Quoted 7 lines

    #include <fcntl.h>

    int main(int argc, char **argv)
    {
        for (int i = 0; i < 10000000; i++)
                open("nonexistent", O_RDONLY);
    }

This reminded me I can see should_failslab() in the profiles (1.43% plus the overhead in its caller) even if it does nothing at all, and it's completely unconditional since commit 4f6923fbb352 ("mm: make should_failslab always available for fault injection").

We discussed it briefly when Jens tried to change it in [1] to depend on CONFIG_FAILSLAB again. But now I think it should be even possible to leave it always available, but behind a static key. BPF or whoever else uses these error injection hooks would have to track how many users of them are active and manage the static key accordingly. Then it could be always available, but have no overhead when there's no active user? Other similars hooks could benefit from such an approach too?

[1]
https://lore.kernel.org/all/[email protected]/#t

Quoted 45 lines · Shakeel Butt

Quoted 43 lines · Linus Torvalds

Quoted 6 lines · Josh Poimboeuf

That's a good point. If the microbenchmark isn't likely to be even remotely realistic, maybe we should just revert the revert until if/when somebody shows a real world impact.

Linus, any objections to that?

We use SLAB_ACCOUNT for much more common allocations like queued signals, so I would tend to agree with Jeff that it's probably just some not very interesting microbenchmark that shows any file locking effects from SLAB_ALLOC, not any real use.

That said, those benchmarks do matter. It's very easy to say "not relevant in the big picture" and then the end result is that everything is a bit of a pig.

And the regression was absolutely *ENORMOUS*. We're not talking "a few percent". We're talking a 33% regression that caused the revert:

   https://lore.kernel.org/lkml/20210907150757.GE17617@xsang-OptiPlex-9020/

I wish our SLAB_ACCOUNT wasn't such a pig. Rather than account every single allocation, it would be much nicer to account at a bigger granularity, possibly by having per-thread counters first before falling back to the obj_cgroup_charge. Whatever.

It's kind of stupid to have a benchmark that just allocates and deallocates a file lock in quick succession spend lots of time incrementing and decrementing cgroup charges for that repeated alloc/free.

However, that problem with SLAB_ACCOUNT is not the fault of file locking, but more of a slab issue.

End result: I think we should bring in Vlastimil and whoever else is doing SLAB_ACCOUNT things, and have them look at that side.

And then just enable SLAB_ACCOUNT for file locks. But very much look at silly costs in SLAB_ACCOUNT first, at least for trivial "alloc/free" patterns..

Quoted 8 lines

Quoted 4 lines

Vlastimil? Who would be the best person to look at that SLAB_ACCOUNT thing? See commit 3754707bcc3e (Revert "memcg: enable accounting for file lock caches") for the history here.

Roman last looked into optimizing this code path. I suspect mod_objcg_state() to be more costly than obj_cgroup_charge(). I will try to measure this path and see if I can improve it.

It's roughly an equal split between mod_objcg_state() and obj_cgroup_charge(). And each is comparable (by order of magnitude) to the slab allocation cost itself. On the free() path a significant cost comes simple from reading the objcg pointer (it's usually a cache miss).

So I don't see how we can make it really cheap (say, less than 5% overhead) without caching pre-accounted objects.

I thought about merging of charge and stats handling paths, which _maybe_ can shave off another 20-30%, but there still will be a double-digit% accounting overhead.

I'm curious to hear other ideas and suggestions.

Thanks!

Quoted 6 lines · Josh Poimboeuf

That's a good point. If the microbenchmark isn't likely to be even remotely realistic, maybe we should just revert the revert until if/when somebody shows a real world impact.

Linus, any objections to that?

We use SLAB_ACCOUNT for much more common allocations like queued signals, so I would tend to agree with Jeff that it's probably just some not very interesting microbenchmark that shows any file locking effects from SLAB_ALLOC, not any real use.

That said, those benchmarks do matter. It's very easy to say "not relevant in the big picture" and then the end result is that everything is a bit of a pig.

And the regression was absolutely *ENORMOUS*. We're not talking "a few percent". We're talking a 33% regression that caused the revert:

   https://lore.kernel.org/lkml/20210907150757.GE17617@xsang-OptiPlex-9020/

I wish our SLAB_ACCOUNT wasn't such a pig. Rather than account every single allocation, it would be much nicer to account at a bigger granularity, possibly by having per-thread counters first before falling back to the obj_cgroup_charge. Whatever.

It's kind of stupid to have a benchmark that just allocates and deallocates a file lock in quick succession spend lots of time incrementing and decrementing cgroup charges for that repeated alloc/free.

However, that problem with SLAB_ACCOUNT is not the fault of file locking, but more of a slab issue.

End result: I think we should bring in Vlastimil and whoever else is doing SLAB_ACCOUNT things, and have them look at that side.

And then just enable SLAB_ACCOUNT for file locks. But very much look at silly costs in SLAB_ACCOUNT first, at least for trivial "alloc/free" patterns..

Vlastimil? Who would be the best person to look at that SLAB_ACCOUNT thing? See commit 3754707bcc3e (Revert "memcg: enable accounting for file lock caches") for the history here.

Linus

Quoted 6 lines · Josh Poimboeuf

That's a good point. If the microbenchmark isn't likely to be even remotely realistic, maybe we should just revert the revert until if/when somebody shows a real world impact.

Linus, any objections to that?

We use SLAB_ACCOUNT for much more common allocations like queued signals, so I would tend to agree with Jeff that it's probably just some not very interesting microbenchmark that shows any file locking effects from SLAB_ALLOC, not any real use.

That said, those benchmarks do matter. It's very easy to say "not relevant in the big picture" and then the end result is that everything is a bit of a pig.

And the regression was absolutely *ENORMOUS*. We're not talking "a few percent". We're talking a 33% regression that caused the revert:

   https://lore.kernel.org/lkml/20210907150757.GE17617@xsang-OptiPlex-9020/

I wish our SLAB_ACCOUNT wasn't such a pig. Rather than account every single allocation, it would be much nicer to account at a bigger granularity, possibly by having per-thread counters first before falling back to the obj_cgroup_charge. Whatever.

It's kind of stupid to have a benchmark that just allocates and deallocates a file lock in quick succession spend lots of time incrementing and decrementing cgroup charges for that repeated alloc/free.

However, that problem with SLAB_ACCOUNT is not the fault of file locking, but more of a slab issue.

End result: I think we should bring in Vlastimil and whoever else is doing SLAB_ACCOUNT things, and have them look at that side.

And then just enable SLAB_ACCOUNT for file locks. But very much look at silly costs in SLAB_ACCOUNT first, at least for trivial "alloc/free" patterns..

Vlastimil? Who would be the best person to look at that SLAB_ACCOUNT thing?

I'm really not a fan of tunables or different kconfig options, especially for something niche like this.

Allow flock cache accounting to be disabled with 'mitigations=off', as it fits the profile for that option: trusted user space combined with a performance-impacting mitigation.