On Thu, 4 Jul 2024 10:45:24 +0200 Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote: > On Thu, Jul 04, 2024 at 10:03:48AM +0200, Peter Zijlstra wrote: > > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > > index c98e3b3386ba..4aafb4485be7 100644 > > --- a/kernel/trace/trace_uprobe.c > > +++ b/kernel/trace/trace_uprobe.c > > @@ -1112,7 +1112,8 @@ static void __probe_event_disable(struct trace_probe *tp) > > if (!tu->inode) > > continue; > > > > - uprobe_unregister(tu->inode, tu->offset, &tu->consumer); > > + uprobe_unregister(tu->inode, tu->offset, &tu->consumer, > > + list_is_last(trace_probe_probe_list(tp), &tu->tp.list) ? 0 : URF_NO_SYNC); > > tu->inode = NULL; > > } > > } > > > Hmm, that continue clause might ruin things. Still easy enough to add > uprobe_unregister_sync() and simpy always pass URF_NO_SYNC. > > I really don't see why we should make this more complicated than it > needs to be. > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > index 354cab634341..681741a51df3 100644 > --- a/include/linux/uprobes.h > +++ b/include/linux/uprobes.h > @@ -115,7 +115,9 @@ extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm > 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_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); > +#define URF_NO_SYNC 0x01 > +extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, unsigned int flags); > +extern void uprobe_unregister_sync(void); > extern int uprobe_mmap(struct vm_area_struct *vma); > extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end); > extern void uprobe_start_dup_mmap(void); > @@ -165,7 +167,7 @@ uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, boo > return -ENOSYS; > } > static inline void > -uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc) > +uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, unsigned int flags) nit: IMHO, I would like to see uprobe_unregister_nosync() variant instead of adding flags. Thank you, > { > } > static inline int uprobe_mmap(struct vm_area_struct *vma) > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 0b7574a54093..d09f7b942076 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -1145,7 +1145,7 @@ __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) > * @offset: offset from the start of the file. > * @uc: identify which probe if multiple probes are colocated. > */ > -void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc) > +void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, unsigned int flags) > { > scoped_guard (srcu, &uprobe_srcu) { > struct uprobe *uprobe = find_uprobe(inode, offset); > @@ -1157,10 +1157,17 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume > mutex_unlock(&uprobe->register_mutex); > } > > - synchronize_srcu(&uprobe_srcu); // XXX amortize / batch > + if (!(flags & URF_NO_SYNC)) > + synchronize_srcu(&uprobe_srcu); > } > EXPORT_SYMBOL_GPL(uprobe_unregister); > > +void uprobe_unregister_sync(void) > +{ > + synchronize_srcu(&uprobe_srcu); > +} > +EXPORT_SYMBOL_GPL(uprobe_unregister_sync); > + > /* > * __uprobe_register - register a probe > * @inode: the file in which the probe has to be placed. > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index d1daeab1bbc1..1f6adabbb1e7 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -3181,9 +3181,10 @@ static void bpf_uprobe_unregister(struct path *path, struct bpf_uprobe *uprobes, > u32 i; > > for (i = 0; i < cnt; i++) { > - uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset, > - &uprobes[i].consumer); > + uprobe_unregister(d_real_inode(path->dentry), uprobes[i].offset, URF_NO_SYNC); > } > + if (cnt > 0) > + uprobe_unregister_sync(); > } > > static void bpf_uprobe_multi_link_release(struct bpf_link *link) > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > index c98e3b3386ba..6b64470a1c5c 100644 > --- a/kernel/trace/trace_uprobe.c > +++ b/kernel/trace/trace_uprobe.c > @@ -1104,6 +1104,7 @@ static int trace_uprobe_enable(struct trace_uprobe *tu, filter_func_t filter) > static void __probe_event_disable(struct trace_probe *tp) > { > struct trace_uprobe *tu; > + bool sync = false; > > tu = container_of(tp, struct trace_uprobe, tp); > WARN_ON(!uprobe_filter_is_empty(tu->tp.event->filter)); > @@ -1112,9 +1113,12 @@ static void __probe_event_disable(struct trace_probe *tp) > if (!tu->inode) > continue; > > - uprobe_unregister(tu->inode, tu->offset, &tu->consumer); > + uprobe_unregister(tu->inode, tu->offset, &tu->consumer, URF_NO_SYNC); > + sync = true; > tu->inode = NULL; > } > + if (sync) > + uprobe_unregister_sync(); > } > > static int probe_event_enable(struct trace_event_call *call, -- Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>