On Wed, 3 Jul 2024 10:13:15 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Mon, Jul 01, 2024 at 03:39:28PM -0700, Andrii Nakryiko wrote:
> > Simplify uprobe registration/unregistration interfaces by making offset
> > and ref_ctr_offset part of uprobe_consumer "interface". In practice, all
> > existing users already store these fields somewhere in uprobe_consumer's
> > containing structure, so this doesn't pose any problem. We just move
> > some fields around.
> >
> > On the other hand, this simplifies uprobe_register() and
> > uprobe_unregister() API by having only struct uprobe_consumer as one
> > thing representing attachment/detachment entity. This makes batched
> > versions of uprobe_register() and uprobe_unregister() simpler.
> >
> > This also makes uprobe_register_refctr() unnecessary, so remove it and
> > simplify consumers.
> >
> > No functional changes intended.
> >
> > Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > include/linux/uprobes.h | 18 +++----
> > kernel/events/uprobes.c | 19 ++-----
> > kernel/trace/bpf_trace.c | 21 +++-----
> > kernel/trace/trace_uprobe.c | 53 ++++++++-----------
> > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 22 ++++----
> > 5 files changed, 55 insertions(+), 78 deletions(-)
> >
> > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > index b503fafb7fb3..a75ba37ce3c8 100644
> > --- 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;
> > };
> >
> > @@ -110,10 +115,9 @@ extern bool is_trap_insn(uprobe_opcode_t *insn);
> > extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs);
> > extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs);
> > extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
> > -extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> > -extern int uprobe_register_refctr(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
> > +extern int uprobe_register(struct inode *inode, struct uprobe_consumer *uc);
> > extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
> > -extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> > +extern void uprobe_unregister(struct inode *inode, struct uprobe_consumer *uc);
>
> It seems very weird and unnatural to split inode and offset like this.
> The whole offset thing only makes sense within the context of an inode.
Hm, so would you mean we should have inode inside the uprobe_consumer?
If so, I think it is reasonable.
Thank you,
>
> So yeah, lets not do this.
--
Masami Hiramatsu
