On Sun, Jul 7, 2024 at 5:50 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> On 07/01, Andrii Nakryiko wrote:
> >
> > --- a/include/linux/uprobes.h
> > +++ b/include/linux/uprobes.h
> > @@ -42,6 +42,11 @@ struct uprobe_consumer {
> > enum uprobe_filter_ctx ctx,
> > struct mm_struct *mm);
> >
> > + /* associated file offset of this probe */
> > + loff_t offset;
> > + /* associated refctr file offset of this probe, or zero */
> > + loff_t ref_ctr_offset;
> > + /* for internal uprobe infra use, consumers shouldn't touch fields below */
> > struct uprobe_consumer *next;
>
>
> Well, I don't really like this patch either...
>
> If nothing else because all the consumers in uprobe->consumers list
> must have the same offset/ref_ctr_offset.
You are thinking from a per-uprobe's perspective. But during
attachment you are attaching multiple consumers at different locations
within a given inode (and that matches for consumers are already
doing, they remember those offsets in their own structs), so each
consumer has a different offset.
Again, I'm just saying that I'm codifying what uprobe users already do
and simplifying the interface (otherwise we'd need another set of
callbacks or some new struct just to pass those
offsets/ref_ctr_offset).
But we can put all that on hold if Peter's approach works well enough.
My goal is to have faster uprobes, not to land *my* patches.
>
> --------------------------------------------------------------------------
> But I agree, the ugly uprobe_register_refctr() must die, we need a single
> function
>
> int uprobe_register(inode, offset, ref_ctr_offset, consumer);
>
> This change is trivial.
>
> --------------------------------------------------------------------------
> And speaking of cleanups, I think another change makes sense:
>
> - int uprobe_register(...);
> + struct uprobe* uprobe_register(...);
>
> so that uprobe_register() returns uprobe or ERR_PTR.
>
> - void uprobe_unregister(inode, offset, consumer);
> + void uprobe_unregister(uprobe, consumer);
>
> this way unregister() doesn't need the extra find_uprobe() + put_uprobe().
> The same for uprobe_apply().
I'm achieving this by keeping uprobe pointer inside uprobe_consumer
(and not requiring callers to keep explicit track of that)
>
> The necessary changes in kernel/trace/trace_uprobe.c are trivial, we just
> need to change struct trace_uprobe
>
> - struct inode *inode;
> + struct uprobe *uprobe;
>
> and fix the compilation errors.
>
>
> As for kernel/trace/bpf_trace.c, I guess struct bpf_uprobe needs the new
> ->uprobe member, we can't kill bpf_uprobe->offset because of
> bpf_uprobe_multi_link_fill_link_info(), but I think this is not that bad.
>
> What do you think?
I'd add an uprobe field to uprobe_consumer, tbh, and keep callers
simpler (less aware of uprobe existence in principle). Even if we
don't do batch register/unregister APIs.
>
> Oleg.
>
