[PATCH 0/2] uprobes: kill xol_area->slot_count

[PATCH 0/2] uprobes: kill xol_area->slot_count

[Oleg Nesterov]

Hello,

On top of

	[PATCH 0/7] uprobes: deuglify xol_get_insn_slot/xol_free_insn_slot paths
	https://lore.kernel.org/all/20240929144201.GA9429@redhat.com/

To me this change looks like a simplification, but perhaps it can
also be considered as optimization.

Yes, in the contended case xol_get_slot_nr() will be called twice,
the second time after prepare_to_wait_event(), but I don't think it
can hurt in the slow path when the caller is likely going to sleep.

Oleg.
---

 kernel/events/uprobes.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

[PATCH 1/2] uprobes: kill xol_area->slot_count

[Oleg Nesterov]

Add the new helper, xol_get_slot_nr() which does
find_first_zero_bit() + test_and_set_bit().

xol_take_insn_slot() can wait for the "xol_get_slot_nr() < UINSNS_PER_PAGE"
event instead of "area->slot_count < UINSNS_PER_PAGE".

So we can kill area->slot_count and avoid atomic_inc() + atomic_dec(), this
simplifies the code and can slightly improve the performance.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 20c58b6ee1ad..38d91e10d455 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -100,7 +100,6 @@ static LIST_HEAD(delayed_uprobe_list);
  */
 struct xol_area {
 	wait_queue_head_t 		wq;		/* if all slots are busy */
-	atomic_t 			slot_count;	/* number of in-use slots */
 	unsigned long 			*bitmap;	/* 0 = free slot */
 
 	struct page			*page;
@@ -1559,7 +1558,6 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
 	init_waitqueue_head(&area->wq);
 	/* Reserve the 1st slot for get_trampoline_vaddr() */
 	set_bit(0, area->bitmap);
-	atomic_set(&area->slot_count, 1);
 	insns = arch_uprobe_trampoline(&insns_size);
 	arch_uprobe_copy_ixol(area->page, 0, insns, insns_size);
 
@@ -1632,24 +1630,28 @@ void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
 	}
 }
 
+static unsigned long xol_get_slot_nr(struct xol_area *area)
+{
+	unsigned long slot_nr;
+
+	slot_nr = find_first_zero_bit(area->bitmap, UINSNS_PER_PAGE);
+	if (slot_nr < UINSNS_PER_PAGE) {
+		if (!test_and_set_bit(slot_nr, area->bitmap))
+			return slot_nr;
+	}
+
+	return UINSNS_PER_PAGE;
+}
+
 /*
  *  - search for a free slot.
  */
 static unsigned long xol_take_insn_slot(struct xol_area *area)
 {
-	unsigned int slot_nr;
+	unsigned long slot_nr;
 
-	for (;;) {
-		slot_nr = find_first_zero_bit(area->bitmap, UINSNS_PER_PAGE);
-		if (slot_nr < UINSNS_PER_PAGE) {
-			if (!test_and_set_bit(slot_nr, area->bitmap))
-				break;
-			continue;
-		}
-		wait_event(area->wq, (atomic_read(&area->slot_count) < UINSNS_PER_PAGE));
-	}
+	wait_event(area->wq, (slot_nr = xol_get_slot_nr(area)) < UINSNS_PER_PAGE);
 
-	atomic_inc(&area->slot_count);
 	return area->vaddr + slot_nr * UPROBE_XOL_SLOT_BYTES;
 }
 
@@ -1685,7 +1687,6 @@ static void xol_free_insn_slot(struct uprobe_task *utask)
 
 	slot_nr = offset / UPROBE_XOL_SLOT_BYTES;
 	clear_bit(slot_nr, area->bitmap);
-	atomic_dec(&area->slot_count);
 	smp_mb__after_atomic(); /* pairs with prepare_to_wait() */
 	if (waitqueue_active(&area->wq))
 		wake_up(&area->wq);
-- 
2.25.1.362.g51ebf55

[PATCH 2/2] uprobes: fold xol_take_insn_slot() into xol_get_insn_slot()

[Oleg Nesterov]

After the previous change xol_take_insn_slot() becomes trivial, kill it.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 38d91e10d455..40ecab0971ff 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1643,29 +1643,20 @@ static unsigned long xol_get_slot_nr(struct xol_area *area)
 	return UINSNS_PER_PAGE;
 }
 
-/*
- *  - search for a free slot.
- */
-static unsigned long xol_take_insn_slot(struct xol_area *area)
-{
-	unsigned long slot_nr;
-
-	wait_event(area->wq, (slot_nr = xol_get_slot_nr(area)) < UINSNS_PER_PAGE);
-
-	return area->vaddr + slot_nr * UPROBE_XOL_SLOT_BYTES;
-}
-
 /*
  * xol_get_insn_slot - allocate a slot for xol.
  */
 static bool xol_get_insn_slot(struct uprobe *uprobe, struct uprobe_task *utask)
 {
 	struct xol_area *area = get_xol_area();
+	unsigned long slot_nr;
 
 	if (!area)
 		return false;
 
-	utask->xol_vaddr = xol_take_insn_slot(area);
+	wait_event(area->wq, (slot_nr = xol_get_slot_nr(area)) < UINSNS_PER_PAGE);
+
+	utask->xol_vaddr = area->vaddr + slot_nr * UPROBE_XOL_SLOT_BYTES;
 	arch_uprobe_copy_ixol(area->page, utask->xol_vaddr,
 			      &uprobe->arch.ixol, sizeof(uprobe->arch.ixol));
 	return true;
-- 
2.25.1.362.g51ebf55

Re: [PATCH 0/2] uprobes: kill xol_area->slot_count

[Andrii Nakryiko]

On Tue, Oct 1, 2024 at 7:24 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> Hello,
>
> On top of
>
>         [PATCH 0/7] uprobes: deuglify xol_get_insn_slot/xol_free_insn_slot paths
>         https://lore.kernel.org/all/20240929144201.GA9429@redhat.com/
>
> To me this change looks like a simplification, but perhaps it can
> also be considered as optimization.
>
> Yes, in the contended case xol_get_slot_nr() will be called twice,
> the second time after prepare_to_wait_event(), but I don't think it
> can hurt in the slow path when the caller is likely going to sleep.
>
> Oleg.
> ---
>
>  kernel/events/uprobes.c | 38 +++++++++++++++++---------------------
>  1 file changed, 17 insertions(+), 21 deletions(-)
>

LGTM, for the series:

Acked-by: Andrii Nakryiko <andrii@kernel.org>


But I've also been wondering if the kernel already has some sort of
slot allocator that we can utilize here, instead of reimplementing all
this? I'll cc Vlastimil in case he has some good pointers (or can
point to someone else who might know).