On Wed, Jul 3, 2024 at 3:13 AM Masami Hiramatsu
<masami.hiramatsu@xxxxxxxxx> wrote:
>
> 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.
>
I don't think so, for at least two reasons.
1) We will be wasting 8 bytes per consumer saving exactly the same
inode pointer for no good reason, while uprobe itself already stores
this inode. One can argue that having offset and ref_ctr_offset inside
uprobe_consumer is wasteful, in principle, and I agree. But all
existing users already store them in the same struct that contains
uprobe_consumer, so we are not regressing anything, while making the
interface simpler. We can always optimize that further, if necessary,
but right now that would give us nothing.
But moving inode into uprobe_consumer will regress memory usage.
2) In the context of batched APIs, offset and ref_ctr_offset varies
with each uprobe_consumer, while inode is explicitly the same for all
consumers in that batch. This API makes it very clear.
Technically, we can remove inode completely from *uprobe_unregister*,
because we now can access it from uprobe_consumer->uprobe->inode, but
it still feels right for symmetry and explicitly making a point that
callers should ensure that inode is kept alive.
> Thank you,
>
> >
> > So yeah, lets not do this.
>
>
> --
> Masami Hiramatsu
