Re: [PATCH v2 08/11] perf/uprobe: Convert (some) uprobe->refcount to SRCU
From: Andrii Nakryiko <hidden>
Date: 2024-07-12 21:21:47
Also in:
bpf, lkml
+ bpf On Thu, Jul 11, 2024 at 4:07 AM Peter Zijlstra [off-list ref] wrote:
quoted hunk ↗ jump to hunk
With handle_swbp() hitting concurrently on (all) CPUs, potentially on the same uprobe, the uprobe->refcount can get *very* hot. Move the struct uprobe lifetime into uprobes_srcu such that it covers both the uprobe and the uprobe->consumers list. With this, handle_swbp() can use a single large SRCU critical section to avoid taking a refcount on the uprobe for it's duration. Notably, the single-step and uretprobe paths need a reference that leaves handle_swbp() and will, for now, still use ->refcount. Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- kernel/events/uprobes.c | 68 ++++++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 27 deletions(-)--- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c@@ -51,7 +51,7 @@ static struct mutex uprobes_mmap_mutex[U DEFINE_STATIC_PERCPU_RWSEM(dup_mmap_sem);
[...]
quoted hunk ↗ jump to hunk
@@ -1982,22 +1990,31 @@ pre_ssout(struct uprobe *uprobe, struct if (!utask) return -ENOMEM; + utask->active_uprobe = try_get_uprobe(uprobe); + if (!utask->active_uprobe) + return -ESRCH; + xol_vaddr = xol_get_insn_slot(uprobe); - if (!xol_vaddr) - return -ENOMEM; + if (!xol_vaddr) { + err = -ENOMEM; + goto err_uprobe; + } utask->xol_vaddr = xol_vaddr; utask->vaddr = bp_vaddr; err = arch_uprobe_pre_xol(&uprobe->arch, regs); - if (unlikely(err)) { - xol_free_insn_slot(current);
let's keep this here, because you later remove err_uprobe part and err_xol is only jumped to from here; it's better to just drop err_xol and err_uprobe altogether and keep the original xol_free_insn_slot() here.
- return err;
- }
+ if (unlikely(err))
+ goto err_xol;
- utask->active_uprobe = uprobe;
utask->state = UTASK_SSTEP;
return 0;
+
+err_xol:
+ xol_free_insn_slot(current);
+err_uprobe:
+ put_uprobe(utask->active_uprobe);utask->active_uprobe = NULL; let's not leave garbage in utask (even if you remove this later anyways)
+ return err; } /*
[...]