On Fri, Aug 2, 2024 at 8:05 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Aug 1, 2024 at 7:41 PM Liao, Chang <liaochang1@xxxxxxxxxx> wrote:
> >
> >
> >
> > 在 2024/8/1 5:42, Andrii Nakryiko 写道:
> > > From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > >
> > > With uprobe_unregister() having grown a synchronize_srcu(), it becomes
> > > fairly slow to call. Esp. since both users of this API call it in a
> > > loop.
> > >
> > > Peel off the sync_srcu() and do it once, after the loop.
> > >
> > > With recent uprobe_register()'s error handling reusing full
> > > uprobe_unregister() call, we need to be careful about returning to the
> > > caller before we have a guarantee that partially attached consumer won't
> > > be called anymore. So add uprobe_unregister_sync() in the error handling
> > > path. This is an unlikely slow path and this should be totally fine to
> > > be slow in the case of an failed attach.
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > > Co-developed-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > ---
> > > include/linux/uprobes.h | 8 ++++++--
> > > kernel/events/uprobes.c | 18 ++++++++++++++----
> > > kernel/trace/bpf_trace.c | 5 ++++-
> > > kernel/trace/trace_uprobe.c | 6 +++++-
> > > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 3 ++-
> > > 5 files changed, 31 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > > index a1686c1ebcb6..8f1999eb9d9f 100644
> > > --- a/include/linux/uprobes.h
> > > +++ b/include/linux/uprobes.h
> > > @@ -105,7 +105,8 @@ 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 struct uprobe *uprobe_register(struct inode *inode, loff_t offset, loff_t ref_ctr_offset, struct uprobe_consumer *uc);
> > > extern int uprobe_apply(struct uprobe *uprobe, struct uprobe_consumer *uc, bool);
> > > -extern void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc);
> > > +extern void uprobe_unregister_nosync(struct uprobe *uprobe, struct uprobe_consumer *uc);
> > > +extern void uprobe_unregister_sync(void);
> >
> > [...]
> >
> > > static inline void
> > > -uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> > > +uprobe_unregister_nosync(struct uprobe *uprobe, struct uprobe_consumer *uc)
> > > +{
> > > +}
> > > +static inline void uprobes_unregister_sync(void)
> >
> > *uprobes*_unregister_sync, is it a typo?
> >
>
> I think the idea behind this is that you do a lot of individual uprobe
> unregistrations with multiple uprobe_unregister() calls, and then
> follow with a single *uprobes*_unregister_sync(), because in general
> it is meant to sync multiple uprobes unregistrations.
Ah, I think you were trying to say that only static inline
implementation here is called uprobes_unregister_sync, while all the
other ones are uprobe_unregister_sync(). I fixed it up, kept it as
singular uprobe_unregister_sync().
>
> > > {
> > > }
> > > static inline int uprobe_mmap(struct vm_area_struct *vma)
> > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > > index 3b42fd355256..b0488d356399 100644
> > > --- a/kernel/events/uprobes.c
> > > +++ b/kernel/events/uprobes.c
[...]
