This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty
- From: Masami Hiramatsu <mhiramat at kernel dot org>
- To: Ingo Molnar <mingo at kernel dot org>
- Cc: Steven Rostedt <rostedt at goodmis dot org>, Ingo Molnar <mingo at redhat dot com>, Alban Crequy <alban dot crequy at gmail dot com>, Alban Crequy <alban at kinvolk dot io>, Alexei Starovoitov <ast at kernel dot org>, Jonathan Corbet <corbet at lwn dot net>, Arnaldo Carvalho de Melo <acme at redhat dot com>, Omar Sandoval <osandov at fb dot com>, linux-doc at vger dot kernel dot org, netdev at vger dot kernel dot org, linux-kernel at vger dot kernel dot org, iago at kinvolk dot io, michael at kinvolk dot io, Dorau Lukasz <lukasz dot dorau at intel dot com>, systemtap at sourceware dot org
- Date: Thu, 30 Mar 2017 22:01:34 +0900
- Subject: Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty
- Authentication-results: sourceware.org; auth=none
- References: <149076484118.24574.7083269903420611708.stgit@devbox> <149076498222.24574.679546540523044200.stgit@devbox> <20170329063005.GA12220@gmail.com> <20170329172510.e012406497fd38a54d5069b3@kernel.org> <20170330065332.GA30148@gmail.com>
On Thu, 30 Mar 2017 08:53:32 +0200
Ingo Molnar <mingo@kernel.org> wrote:
>
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > > So this is something I missed while the original code was merged, but the concept
> > > looks a bit weird: why do we do any "allocation" while a handler is executing?
> > >
> > > That's fundamentally fragile. What's the maximum number of parallel
> > > 'kretprobe_instance' required per kretprobe - one per CPU?
> >
> > It depends on the place where we put the probe. If the probed function will be
> > blocked (yield to other tasks), then we need a same number of threads on
> > the system which can invoke the function. So, ultimately, it is same
> > as function_graph tracer, we need it for each thread.
>
> So then put it into task_struct (assuming there's no kretprobe-inside-kretprobe
> nesting allowed).
No, that is possible to put several kretprobes on same thread, e.g.
the func1() is called from func2(), user can put kretprobes for each
function at same time.
So the possible solution is to allocate new return-stack for each task_struct,
and that is what the function-graph tracer did.
Anyway, I'm considering to integrate kretprobe_instance with the ret_stack.
It will increase memory usage for kretprobes, but can provide safer way
to allocate kretprobe_instance.
> There's just no way in hell we should be calling any complex
> kernel function from kernel probes!
OK, so let's drop this, since it may easily cause deadlock...
> I mean, think about it, a kretprobe can be installed in a lot of places, and now
> we want to call get_free_pages() from it?? This would add a massive amount of
> fragility.
I thought it was safe because GFP_ATOMIC is safe at interrupt handler.
> Instrumentation must be _simple_, every patch that adds more complexity to the
> most fundamental code path of it should raise a red flag ...
>
> So let's make this more robust, ok?
Yeah, in that case, I think Alban's patch is enough at this point since
it gives user to tune their kretprobe events not to be missed.
Thank you,
>
> Thanks,
>
> Ingo
--
Masami Hiramatsu <mhiramat@kernel.org>