Re: [PATCH] mm: don't call should_failslab() for !CONFIG_FAILSLAB

[Vlastimil Babka]

On 5/27/24 11:34 AM, Mateusz Guzik wrote:
> +cc Linus
> 
> On Thu, Oct 07, 2021 at 05:32:52PM +0200, Vlastimil Babka wrote:
>> On 10/5/21 17:31, Jens Axboe wrote:
>> > Allocations can be a very hot path, and this out-of-line function
>> > call is noticeable.
>> > 
>> > Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
>> 
>> It used to be inline b4 (hi, Konstantin!) and then was converted to be like
>> this intentionally :/
>> 
>> See 4f6923fbb352 ("mm: make should_failslab always available for fault
>> injection")
>> 
>> And now also kernel/bpf/verifier.c contains:
>> BTF_ID(func, should_failslab)
>> 
>> I think either your or Andrew's version will break this BTF_ID thing, at the
>> very least.
>> 
>> But I do strongly agree that putting unconditionally a non-inline call into
>> slab allocator fastpath sucks. Can we make it so that bpf can only do these
>> overrides when CONFIG_FAILSLAB is enabled?
>> I don't know, perhaps putting this BTF_ID() in #ifdef as well, or providing
>> a dummy that is always available (so that nothing breaks), but doesn't
>> actually affect slab_pre_alloc_hook() unless CONFIG_FAILSLAB has been enabled?
>> 
> 
> I just ran into it while looking at kmalloc + kfree pair.
> 
> A toy test which calls this in a loop like so:
> static long noinline custom_bench(void)
> {
>         void *buf;
> 
>         while (!signal_pending(current)) {
>                 buf = kmalloc(16, GFP_KERNEL);
>                 kfree(buf);
>                 cond_resched();
>         }
> 
>         return -EINTR;
> }
> 
> ... shows this with perf top:
>    57.88%  [kernel]           [k] kfree
>    31.38%  [kernel]           [k] kmalloc_trace_noprof
>     3.20%  [kernel]           [k] should_failslab.constprop.0
> 
> A side note is that I verified majority of the time in kfree and
> kmalloc_trace_noprof is cmpxchg16b, which is both good and bad news.
> 
> As for should_failslab, it compiles to an empty func on production
> kernels and is present even when there are no supported means of
> instrumenting it. As in everyone pays for its existence, even if there
> is no way to use it.
> 
> Also note there are 3 unrelated mechanisms to alter the return code,
> which imo is 2 too many. But more importantly they are not even
> coordinated.
> 
> A hard requirement for a long term solution is to not alter the fast
> path beyond nops for hot patching.
> 
> So far I think implementing this in a clean manner would require
> agreeing on some namespace for bpf ("failprobes"?) and coordinating
> hotpatching between different mechanisms. Maybe there is a better, I
> don't know.

I've attempted something (not complete yet) here:

https://lore.kernel.org/all/20240531-fault-injection-statickeys-v1-0-a513fd0a9614@xxxxxxx/

> Here is the crux of my e-mail though:
> 1. turning should_failslab into a mandatory func call is an ok local
>    hack for the test farm, not a viable approach for production
> 2. as such it is up to the original submitter (or whoever else
>    who wants to pick up the slack) to implement something which
>    hotpatches the callsite as opposed to inducing a function call for
>    everyone
> 
> In the meantime the routine should disappear unless explicitly included
> in kernel config. The patch submitted here would be one way to do it.