Re: [PATCH v2 05/12] uprobes: move offset and ref_ctr_offset into uprobe_consumer

[Masami Hiramatsu <masami.hiramatsu@gmail.com>]

On Wed, 3 Jul 2024 10:13:15 +0200
Peter Zijlstra <peterz@infradead.org> 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@kernel.org>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  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