Re: [PATCH] bpf: don't fail kmalloc while releasing raw_tp

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 16 Nov 2020 15:37:27 -0500 (EST)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > 
> > Mathieu,
> > 
> > Can't we do something that would still allow to unregister a probe even if
> > a new probe array fails to allocate? We could kick off a irq work to try to
> > clean up the probe at a later time, but still, the unregister itself should
> > not fail due to memory failure.  
> 
> Currently, the fast path iteration looks like:
> 
>                 struct tracepoint_func *it_func_ptr;
>                 void *it_func;
> 
>                 it_func_ptr =                                           \
>                         rcu_dereference_raw((&__tracepoint_##_name)->funcs); \
>                 do {                                                    \
>                         it_func = (it_func_ptr)->func;                  \
>                         __data = (it_func_ptr)->data;                   \
>                         ((void(*)(void *, proto))(it_func))(__data, args); \
>                 } while ((++it_func_ptr)->func); 
> 
> So we RCU dereference the array, and iterate on the array until we find a NULL
> func. So you could not use NULL to skip items, but you could perhaps reserve
> a (void *)0x1UL tombstone for this.

Actually, you could just set it to a stub callback that does nothing. then
you don't even need to touch the above macro. Not sure why I didn't
recommend this to begin with, because that's exactly what the function
tracer does with ftrace_stub.


> 
> It should ideally be an unlikely branch, and it would be good to benchmark the
> change when multiple tracing probes are attached to figure out whether the
> overhead is significant when tracing is enabled.

If you use a stub function, it shouldn't affect anything. And the worse
that would happen is that you have a slight overhead of calling the stub
until you can properly remove the callback.

> 
> I wonder whether we really mind that much about using slightly more memory
> than required after a failed reallocation due to ENOMEM. Perhaps the irq work
> is not even needed. Chances are that the irq work would fail again and again if
> it's in low memory conditions. So maybe it's better to just keep the tombstone
> in place until the next successful callback array reallocation.
> 

True. If we just replace the function with a stub on memory failure (always
using __GFP_NOFAIL, and if it fails to reallocate a new array, just replace
the callback with the stub and be done with it. It may require some more
accounting to make sure the tracepoint.c code can handle these stubs, and
remove them on new additions to the code. Heck, if a stub exists, you could
just swap it with a new item. But on any new changes to the list, the stubs
should be purged.


-- Steve