[PATCH v13 09/14] unwind deferred: Use SRCU unwind_deferred_task_work()

Steven Rostedt posted 14 patches 3 months ago
There is a newer version of this series
[PATCH v13 09/14] unwind deferred: Use SRCU unwind_deferred_task_work()
Posted by Steven Rostedt 3 months ago
From: Steven Rostedt <rostedt@goodmis.org>

Instead of using the callback_mutex to protect the link list of callbacks
in unwind_deferred_task_work(), use SRCU instead. This gets called every
time a task exits that has to record a stack trace that was requested.
This can happen for many tasks on several CPUs at the same time. A mutex
is a bottleneck and can cause a bit of contention and slow down performance.

As the callbacks themselves are allowed to sleep, regular RCU cannot be
used to protect the list. Instead use SRCU, as that still allows the
callbacks to sleep and the list can be read without needing to hold the
callback_mutex.

Link: https://lore.kernel.org/all/ca9bd83a-6c80-4ee0-a83c-224b9d60b755@efficios.com/

Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/unwind/deferred.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c
index 5edb648b7de7..9aed9866f460 100644
--- a/kernel/unwind/deferred.c
+++ b/kernel/unwind/deferred.c
@@ -41,10 +41,11 @@ static inline bool try_assign_cnt(struct unwind_task_info *info, u32 cnt)
 #define UNWIND_MAX_ENTRIES					\
 	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
 
-/* Guards adding to and reading the list of callbacks */
+/* Guards adding to or removing from the list of callbacks */
 static DEFINE_MUTEX(callback_mutex);
 static LIST_HEAD(callbacks);
 static unsigned long unwind_mask;
+DEFINE_STATIC_SRCU(unwind_srcu);
 
 /*
  * This is a unique percpu identifier for a given task entry context.
@@ -143,6 +144,7 @@ static void unwind_deferred_task_work(struct callback_head *head)
 	struct unwind_stacktrace trace;
 	struct unwind_work *work;
 	u64 cookie;
+	int idx;
 
 	if (WARN_ON_ONCE(!local_read(&info->pending)))
 		return;
@@ -161,13 +163,15 @@ static void unwind_deferred_task_work(struct callback_head *head)
 
 	cookie = info->id.id;
 
-	guard(mutex)(&callback_mutex);
-	list_for_each_entry(work, &callbacks, list) {
+	idx = srcu_read_lock(&unwind_srcu);
+	list_for_each_entry_srcu(work, &callbacks, list,
+				 srcu_read_lock_held(&unwind_srcu)) {
 		if (test_bit(work->bit, &info->unwind_mask)) {
 			work->func(work, &trace, cookie);
 			clear_bit(work->bit, &info->unwind_mask);
 		}
 	}
+	srcu_read_unlock(&unwind_srcu, idx);
 }
 
 /**
@@ -199,6 +203,7 @@ int unwind_deferred_request(struct unwind_work *work, u64 *cookie)
 {
 	struct unwind_task_info *info = &current->unwind_info;
 	long pending;
+	int bit;
 	int ret;
 
 	*cookie = 0;
@@ -211,12 +216,17 @@ int unwind_deferred_request(struct unwind_work *work, u64 *cookie)
 	if (!CAN_USE_IN_NMI && in_nmi())
 		return -EINVAL;
 
+	/* Do not allow cancelled works to request again */
+	bit = READ_ONCE(work->bit);
+	if (WARN_ON_ONCE(bit < 0))
+		return -EINVAL;
+
 	guard(irqsave)();
 
 	*cookie = get_cookie(info);
 
 	/* This is already queued */
-	if (test_bit(work->bit, &info->unwind_mask))
+	if (test_bit(bit, &info->unwind_mask))
 		return 1;
 
 	/* callback already pending? */
@@ -240,25 +250,32 @@ int unwind_deferred_request(struct unwind_work *work, u64 *cookie)
 	}
 
  out:
-	return test_and_set_bit(work->bit, &info->unwind_mask);
+	return test_and_set_bit(bit, &info->unwind_mask);
 }
 
 void unwind_deferred_cancel(struct unwind_work *work)
 {
 	struct task_struct *g, *t;
+	int bit;
 
 	if (!work)
 		return;
 
 	guard(mutex)(&callback_mutex);
-	list_del(&work->list);
+	list_del_rcu(&work->list);
+	bit = work->bit;
+
+	/* Do not allow any more requests and prevent callbacks */
+	work->bit = -1;
+
+	__clear_bit(bit, &unwind_mask);
 
-	__clear_bit(work->bit, &unwind_mask);
+	synchronize_srcu(&unwind_srcu);
 
 	guard(rcu)();
 	/* Clear this bit from all threads */
 	for_each_process_thread(g, t) {
-		clear_bit(work->bit, &t->unwind_info.unwind_mask);
+		clear_bit(bit, &t->unwind_info.unwind_mask);
 	}
 }
 
@@ -275,7 +292,7 @@ int unwind_deferred_init(struct unwind_work *work, unwind_callback_t func)
 	work->bit = ffz(unwind_mask);
 	__set_bit(work->bit, &unwind_mask);
 
-	list_add(&work->list, &callbacks);
+	list_add_rcu(&work->list, &callbacks);
 	work->func = func;
 	return 0;
 }
-- 
2.47.2
Re: [PATCH v13 09/14] unwind deferred: Use SRCU unwind_deferred_task_work()
Posted by Peter Zijlstra 2 months, 3 weeks ago
On Mon, Jul 07, 2025 at 09:22:48PM -0400, Steven Rostedt wrote:

> @@ -143,6 +144,7 @@ static void unwind_deferred_task_work(struct callback_head *head)
>  	struct unwind_stacktrace trace;
>  	struct unwind_work *work;
>  	u64 cookie;
> +	int idx;
>  
>  	if (WARN_ON_ONCE(!local_read(&info->pending)))
>  		return;
> @@ -161,13 +163,15 @@ static void unwind_deferred_task_work(struct callback_head *head)
>  
>  	cookie = info->id.id;
>  
> -	guard(mutex)(&callback_mutex);
> -	list_for_each_entry(work, &callbacks, list) {
> +	idx = srcu_read_lock(&unwind_srcu);
> +	list_for_each_entry_srcu(work, &callbacks, list,
> +				 srcu_read_lock_held(&unwind_srcu)) {
>  		if (test_bit(work->bit, &info->unwind_mask)) {
>  			work->func(work, &trace, cookie);
>  			clear_bit(work->bit, &info->unwind_mask);
>  		}
>  	}
> +	srcu_read_unlock(&unwind_srcu, idx);
>  }

Please; something like so:

--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -524,4 +524,9 @@ DEFINE_LOCK_GUARD_1(srcu, struct srcu_st
 		    srcu_read_unlock(_T->lock, _T->idx),
 		    int idx)
 
+DEFINE_LOCK_GUARD_1(srcu_lite, struct srcu_struct,
+		    _T->idx = srcu_read_lock_lite(_T->lock),
+		    srcu_read_unlock_lite(_T->lock, _T->idx),
+		    int idx)
+
 #endif
--- a/kernel/unwind/deferred.c
+++ b/kernel/unwind/deferred.c
@@ -165,7 +165,7 @@ static void unwind_deferred_task_work(st
 
 	cookie = info->id.id;
 
-	guard(mutex)(&callback_mutex);
+	guard(srcu_lite)(&unwind_srcu);
 	list_for_each_entry(work, &callbacks, list) {
 		work->func(work, &trace, cookie);
 	}
Re: [PATCH v13 09/14] unwind deferred: Use SRCU unwind_deferred_task_work()
Posted by Steven Rostedt 2 months, 3 weeks ago
On Mon, 14 Jul 2025 15:56:38 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Please; something like so:
> 
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -524,4 +524,9 @@ DEFINE_LOCK_GUARD_1(srcu, struct srcu_st
>  		    srcu_read_unlock(_T->lock, _T->idx),
>  		    int idx)
>  
> +DEFINE_LOCK_GUARD_1(srcu_lite, struct srcu_struct,
> +		    _T->idx = srcu_read_lock_lite(_T->lock),
> +		    srcu_read_unlock_lite(_T->lock, _T->idx),
> +		    int idx)
> +
>  #endif
> --- a/kernel/unwind/deferred.c
> +++ b/kernel/unwind/deferred.c
> @@ -165,7 +165,7 @@ static void unwind_deferred_task_work(st
>  
>  	cookie = info->id.id;
>  
> -	guard(mutex)(&callback_mutex);
> +	guard(srcu_lite)(&unwind_srcu);
>  	list_for_each_entry(work, &callbacks, list) {
>  		work->func(work, &trace, cookie);
>  	}

I think I rather have a scoped_guard() here. One thing that bothers me
about the guard() logic is that it could easily start to "leak"
protection. That is, the unwind_srcu is only needed for walking the
list. The reason I chose to open code the protection, is because I
wanted to distinctly denote where the end of the protection was.

-- Steve
Re: [PATCH v13 09/14] unwind deferred: Use SRCU unwind_deferred_task_work()
Posted by Peter Zijlstra 2 months, 3 weeks ago
On Mon, Jul 14, 2025 at 10:21:40AM -0400, Steven Rostedt wrote:
> On Mon, 14 Jul 2025 15:56:38 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Please; something like so:
> > 
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -524,4 +524,9 @@ DEFINE_LOCK_GUARD_1(srcu, struct srcu_st
> >  		    srcu_read_unlock(_T->lock, _T->idx),
> >  		    int idx)
> >  
> > +DEFINE_LOCK_GUARD_1(srcu_lite, struct srcu_struct,
> > +		    _T->idx = srcu_read_lock_lite(_T->lock),
> > +		    srcu_read_unlock_lite(_T->lock, _T->idx),
> > +		    int idx)
> > +
> >  #endif
> > --- a/kernel/unwind/deferred.c
> > +++ b/kernel/unwind/deferred.c
> > @@ -165,7 +165,7 @@ static void unwind_deferred_task_work(st
> >  
> >  	cookie = info->id.id;
> >  
> > -	guard(mutex)(&callback_mutex);
> > +	guard(srcu_lite)(&unwind_srcu);
> >  	list_for_each_entry(work, &callbacks, list) {
> >  		work->func(work, &trace, cookie);
> >  	}
> 
> I think I rather have a scoped_guard() here. One thing that bothers me
> about the guard() logic is that it could easily start to "leak"
> protection. That is, the unwind_srcu is only needed for walking the
> list. The reason I chose to open code the protection, is because I
> wanted to distinctly denote where the end of the protection was.

Sure. But the point was more to:
 - use scru_lite; and,
 - use guards