Re: [RESEND][PATCH v31 8/9] sched: Add deactivated (sleeping) owner handling to find_proxy_task()

[Peter Zijlstra <peterz@infradead.org>]

I'm slowly working my way through this one..

On Fri, Aug 07, 2026 at 03:52:14AM +0000, John Stultz wrote:

> +static inline void proxy_remove_from_sleeping_owner(struct task_struct *p)
> +{
> +	struct task_struct *owner = READ_ONCE(p->sleeping_owner);
> +
> +	if (owner) {
> +		/*
> +		 * __proxy_remove_from_sleeping_owner() does a
> +		 * put on owner to match the get done in
> +		 * proxy_enqueue_on_owner(). If that put is the
> +		 * last one and it frees owner, we'd be freeing
> +		 * a lock we held. So get/put owner around its
> +		 * usage her to ensure that doesn't happen.
> +		 */
> +		get_task_struct(owner);
> +		raw_spin_lock(&owner->blocked_lock);
> +		__proxy_remove_from_sleeping_owner(owner, p);
> +		raw_spin_unlock(&owner->blocked_lock);
> +		put_task_struct(owner);
> +	}
> +}

I am not convinced this is correct. Notably
__proxy_remove_from_sleeping_owner() does nothing if p->sleeping_owner
!= owner.

Additionally, owner is read without serialization.

Either owner can change, and you need to:
 - verify p->sleeping_owner == owner once you've acquired
   owner->blocked_lock, and retry if it doesn't match
or
 - explain that once ->sleeping_owner is !NULL, it cannot change and
   instead of NOP-ing out when ->sleeping_owner != owner, assert that it
   is.


> @@ -6852,6 +7078,29 @@ static void proxy_migrate_task(struct rq *rq, struct rq_flags *rf,
>  	proxy_reacquire_rq_lock(rq, rf);
>  }
>  
> +static void proxy_enqueue_on_owner(struct rq *rq, struct task_struct *owner,
> +				   struct task_struct *p)
> +{
> +	lockdep_assert_rq_held(rq);
> +	lockdep_assert_held(&owner->blocked_lock);
> +	/*
> +	 * ttwu_activate() will pick them up and place them on whatever rq
> +	 * @owner will run next.
> +	 */
> +	WARN_ON(p == owner);
> +	WARN_ON(!p->on_rq);
> +	WARN_ON(p->sleeping_owner);
> +	get_task_struct(owner);
> +	WRITE_ONCE(p->sleeping_owner, owner);
> +	/*
> +	 * ttwu_do_activate must not have a chance to activate p
> +	 * elsewhere before it's fully extricated from its old rq.
> +	 */
> +	list_add(&p->blocked_node, &owner->blocked_head);
> +	proxy_resched_idle(rq);
> +	block_task(rq, p, READ_ONCE(p->__state));
> +}

Initially I thought you needed: smp_store_release(&p->sleeping_owner =
owner); after the list_add, and a corresponding smp_load_acquire() in
the function above. However since they both acquire ->blocked_lock
(eventually) this works out. But it might want to have a comment along
those lines.

Random changed that happened whilst I was going over things.

---

--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1261,11 +1261,11 @@ struct task_struct {
 	struct mutex			*blocked_on;	/* lock we're blocked on */
 	raw_spinlock_t			blocked_lock;
 #ifdef CONFIG_SCHED_PROXY_EXEC
-	struct list_head		blocked_head;  /* tasks blocked on this task */
+	struct list_head		blocked_head __guarded_by(&blocked_lock); /* tasks blocked on this task */
 	struct list_head		blocked_node;  /* our entry on someone elses blocked_head */
 	/* Node for list of tasks to process blocked_head list for blocked entitiy activations */
 	struct list_head		blocked_activation_node;
-	struct task_struct		*sleeping_owner; /* task our blocked_node is enqueued on */
+	struct task_struct		*sleeping_owner __guarded_by(&blocked_lock); /* task our blocked_node is enqueued on */
 #endif
 
 	/*
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -160,6 +160,7 @@ static inline void put_task_struct(struc
 }
 
 DEFINE_FREE(put_task, struct task_struct *, if (_T) put_task_struct(_T))
+DEFINE_GUARD(get_task, struct task_struct *, get_task_struct(_T), put_task_struct(_T))
 
 static inline void put_task_struct_many(struct task_struct *t, int nr)
 {
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2242,33 +2242,33 @@ void __proxy_remove_from_sleeping_owner(
 
 static inline void proxy_remove_from_sleeping_owner(struct task_struct *p)
 {
-	struct task_struct *owner = READ_ONCE(p->sleeping_owner);
+	struct task_struct *owner = data_race(READ_ONCE(p->sleeping_owner));
 
-	if (owner) {
-		/*
-		 * __proxy_remove_from_sleeping_owner() does a
-		 * put on owner to match the get done in
-		 * proxy_enqueue_on_owner(). If that put is the
-		 * last one and it frees owner, we'd be freeing
-		 * a lock we held. So get/put owner around its
-		 * usage her to ensure that doesn't happen.
-		 */
-		get_task_struct(owner);
-		raw_spin_lock(&owner->blocked_lock);
-		__proxy_remove_from_sleeping_owner(owner, p);
-		raw_spin_unlock(&owner->blocked_lock);
-		put_task_struct(owner);
-	}
+	if (!owner)
+		return;
+
+	/*
+	 * __proxy_remove_from_sleeping_owner() does a
+	 * put on owner to match the get done in
+	 * proxy_enqueue_on_owner(). If that put is the
+	 * last one and it frees owner, we'd be freeing
+	 * a lock we held. So get/put owner around its
+	 * usage her to ensure that doesn't happen.
+	 */
+	guard(get_task)(owner);
+	guard(raw_spinlock)(&owner->blocked_lock);
+	__proxy_remove_from_sleeping_owner(owner, p);
 }
 
 void activate_task(struct rq *rq, struct task_struct *p, int en_flags)
 {
+	lockdep_assert_rq_held(rq);
+
 	if (!sched_proxy_exec()) {
 		__activate_task(rq, p, en_flags);
 		return;
 	}
 
-	lockdep_assert_rq_held(rq);
 	proxy_remove_from_sleeping_owner(p);
 	/*
 	 * By calling __activate_task() with blocked_lock held, we
@@ -2276,10 +2276,9 @@ void activate_task(struct rq *rq, struct
 	 * such that no more blocked tasks will be enqueued on p
 	 * once we release p->blocked_lock.
 	 */
-	raw_spin_lock(&p->blocked_lock);
+	guard(raw_spinlock)(&p->blocked_lock);
 	WARN_ON(task_cpu(p) != cpu_of(rq));
 	__activate_task(rq, p, en_flags);
-	raw_spin_unlock(&p->blocked_lock);
 }
 #else
 static inline void proxy_remove_from_sleeping_owner(struct task_struct *p)