On Fri, Jul 05, 2024 at 05:35:44PM +0900, Masami Hiramatsu wrote:
SNIP
> > > > > Also, if we can set session enabled by default, and skip ret_handler by handler's
> > > > > return value, it is more simpler. (If handler returns a specific value, skip ret_handler)
> > > >
> > > > you mean derive if it's a session or not by both handler and
> > > > ret_handler being set? I guess this works fine for BPF side, because
> > > > there we never had them both set. If this doesn't regress others, I
> > > > think it's OK. We just need to make sure we don't unnecessarily
> > > > allocate session state for consumers that don't set both handler and
> > > > ret_handler. That would be a waste.
> > >
> > > hum.. so the current code installs return uprobe if there's ret_handler
> > > defined in consumer and also the entry 'handler' needs to return 0
> > >
> > > if entry 'handler' returns 1 the uprobe is unregistered
> > >
> > > we could define new return value from 'handler' to 'not execute the
> > > 'ret_handler' and have 'handler' return values:
> > >
> > > 0 - execute 'ret_handler' if defined
> > > 1 - remove the uprobe
> > > 2 - do NOT execute 'ret_handler' // this current triggers WARN
> > >
> > > we could delay the allocation of 'return_instance' until the first
> > > consumer returns 0, so there's no perf regression
> > >
> > > that way we could treat all consumers the same and we wouldn't need
> > > the session flag..
> > >
> > > ok looks like good idea ;-) will try that
> >
> > Just please double check that we don't pass through 1 or 2 as a return
> > result for BPF uprobes/multi-uprobes, so that we don't have any
> > accidental changes of behavior.
>
> Agreed. BTW, even if the uprobe is removed, the ret_handler should be called?
> I think both 1 and 2 case, we should skip ret_handler.
do you mean what happens when the uretprobe is installed and its consumer
is unregistered before it's triggered?
I think it won't get executed, because the consumer is removed right away,
even if the uprobe object stays because the return_instance holds ref to it
>
> > > > > >
> > > > > > #ifdef CONFIG_UPROBES
> > > > > > @@ -80,6 +83,12 @@ struct uprobe_task {
> > > > > > unsigned int depth;
> > > > > > };
> > > > > >
> > > > > > +struct session_consumer {
> > > > > > + __u64 cookie;
> > > > >
> > > > > And this cookie looks not scalable. If we can pass a data to handler, I would like to
> > > > > reuse it to pass the target function parameters to ret_handler as kretprobe/fprobe does.
> > > > >
> > > > > int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs, void *data);
> > > > >
> > > > > uprobes can collect its uc's required sizes and allocate the memory (shadow stack frame)
> > > > > at handler_chain().
> > > >
> > > > The goal here is to keep this simple and fast. I'd prefer to keep it
> > > > small and fixed size, if possible. I'm thinking about caching and
> > > > reusing return_instance as one of the future optimizations, so if we
> > > > can keep this more or less fixed (assuming there is typically not more
> > > > than 1 or 2 consumers per uprobe, which seems realistic), this will
> > > > provide a way to avoid excessive memory allocations.
>
> Hmm, so you mean user will allocate another "data map" and use cookie as
> a key to access the data? That is possible but sounds a bit redundant.
> If such "data map" allocation is also provided, it is more useful.
is the argument only about the size of the shared data?
we can make it configurable.. for bpf user we will probably stick to
8 bytes to match the kprobe session and to be compatible with existing
helpers accessing the cookie
jirka
