On 2021/7/11 PM10:09, Masami Hiramatsu wrote:
On Wed, 7 Jul 2021 22:42:47 +0800 Matt Wu <wuqiang.matt@xxxxxxxxxxxxx> wrote:On 2021/7/7 PM9:29, Masami Hiramatsu wrote:On Wed, 7 Jul 2021 19:45:30 +0900 Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:On Wed, 7 Jul 2021 12:20:57 +0200 Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:On Wed, Jul 07, 2021 at 07:15:10PM +0900, Masami Hiramatsu wrote:I actually don't want to keep this feature because no one use it. (only systemtap needs it?)Yeah, you mentioned systemtap, but since that's out-of-tree I don't care. Their problem.Yeah, maybe it is not hard to update.Anyway, if we keep the idea-level compatibility (not code level), what we need is 'void *data' in the struct kretprobe_instance. User who needs it can allocate their own instance data for their kretprobes when initialising it and sets in their entry handler. Then we can have a simple kretprobe_instance.When would you do the alloc? When installing the retprobe, but that might be inside the allocator, which means you can't call the allocator etc.. :-)Yes, so the user may need to allocate a pool right before register_kretprobe(). (whether per-kretprobe or per-task or global pool, that is user's choice.)If we look at struct ftrace_ret_stack, it has a few fixed function fields. The calltime one is all that is needed for the kretprobe example code.kretprobe consumes 3 fields, a pointer to 'struct kretprobe' (which stores callee function address in 'kretprobe::kp.addr'), a return address and a frame pointer (*).> Oops, I forgot to add "void *data" for storing user data.Should use "struct kretprobe_holder *rph", since "struct kretprobe" belongs to 3rd-party module (which might be unloaded any time).Good catch. Yes, instead of 'struct kretprobe', we need to use the holder.User's own pool might not work if the module can be unloaded. Better manage the pool in kretprobe_holder, which needs no changes from user side.No, since the 'data' will be only refered from user handler. If the kretprobe is released, then the kretprobe_holder will clear the refernce to the 'struct kretprobe'. Then, the user handler is never called. No one access the 'data'.
Indeed, there is no race of "data" accessing, since unregister_kretprobes() is taking care of it. This implementation just increases the complexity of caller to keep track of all allocated instances and release them after unregistration. But guys are likely to use kmalloc in pre-handler and kfree in post-handler, which will lead to memory leaks.
|