Date:   Tue, 7 Mar 2023 20:31:22 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     John Stultz <jstultz@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, Wei Wang <wvw@google.com>,
        Midas Chien <midaschieh@google.com>,
        Kees Cook <keescook@chromium.org>,
        Anton Vorontsov <anton@enomsg.org>,
        "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
        Tony Luck <tony.luck@intel.com>, kernel-team@android.com,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Subject: Re: [PATCH] pstore: Revert pmsg_lock back to a normal mutex
Message-ID: <20230307203122.14b077c4@gandalf.local.home>
In-Reply-To: <20230302200136.381468f0@gandalf.local.home>
References: <20230302062741.483079-1-jstultz@google.com>
        <20230302082414.77613351@gandalf.local.home>
        <CANDhNCo4ruC4pP+iDe49b3e1nAcWtYQj4bx82+oZhyLFYkdFJQ@mail.gmail.com>
        <20230302152103.2618f1b7@gandalf.local.home>
        <20230302163253.541ac3a8@gandalf.local.home>
        <20230302163603.223313ba@gandalf.local.home>
        <20230302165613.2dcc18ca@gandalf.local.home>
        <20230302200136.381468f0@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2 Mar 2023 20:01:36 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> @@ -1421,11 +1425,23 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
>  		 *    for CONFIG_PREEMPT_RCU=y)
>  		 *  - the VCPU on which owner runs is preempted
>  		 */
> -		if (!owner_on_cpu(owner) || need_resched() ||
> -		    !rt_mutex_waiter_is_top_waiter(lock, waiter)) {
> +		if (!owner_on_cpu(owner) || need_resched()) {
>  			res = false;
>  			break;
>  		}
> +		top_waiter = rt_mutex_top_waiter(lock);

rt_mutex_top_waiter() can not be called outside the wait_lock, as it may
trigger that BUG_ON() you saw.

New patch below.

> +		if (top_waiter != waiter) {
> +			if (top_waiter != last_waiter) {
> +				raw_spin_lock_irq(&lock->wait_lock);
> +				last_waiter = rt_mutex_top_waiter(lock);
> +				top_task = last_waiter->task;
> +				raw_spin_unlock_irq(&lock->wait_lock);
> +			}
> +			if (!owner_on_cpu(top_task)) {
> +				res = false;
> +				break;
> +			}
> +		}

-- Steve

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 7779ee8abc2a..f7b0cc3be20e 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1362,8 +1362,11 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
 				  struct rt_mutex_waiter *waiter,
 				  struct task_struct *owner)
 {
+	struct rt_mutex_waiter *top_waiter = NULL;
+	struct task_struct *top_task = NULL;
 	bool res = true;
 
+	/* rcu_read_lock keeps task_structs around */
 	rcu_read_lock();
 	for (;;) {
 		/* If owner changed, trylock again. */
@@ -1384,11 +1387,22 @@ static bool rtmutex_spin_on_owner(struct rt_mutex_base *lock,
 		 *    for CONFIG_PREEMPT_RCU=y)
 		 *  - the VCPU on which owner runs is preempted
 		 */
-		if (!owner_on_cpu(owner) || need_resched() ||
-		    !rt_mutex_waiter_is_top_waiter(lock, waiter)) {
+		if (!owner_on_cpu(owner) || need_resched()) {
 			res = false;
 			break;
 		}
+		if (!rt_mutex_waiter_is_top_waiter(lock, waiter)) {
+			if (!rt_mutex_waiter_is_top_waiter(lock, top_waiter)) {
+				raw_spin_lock_irq(&lock->wait_lock);
+				top_waiter = rt_mutex_top_waiter(lock);
+				top_task = top_waiter->task;
+				raw_spin_unlock_irq(&lock->wait_lock);
+			}
+			if (!owner_on_cpu(top_task)) {
+				res = false;
+				break;
+			}
+		}
 		cpu_relax();
 	}
 	rcu_read_unlock();
@@ -1510,10 +1524,7 @@ static int __sched rt_mutex_slowlock_block(struct rt_mutex_base *lock,
 				break;
 		}
 
-		if (waiter == rt_mutex_top_waiter(lock))
-			owner = rt_mutex_owner(lock);
-		else
-			owner = NULL;
+		owner = rt_mutex_owner(lock);
 		raw_spin_unlock_irq(&lock->wait_lock);
 
 		if (!owner || !rtmutex_spin_on_owner(lock, waiter, owner))
@@ -1699,10 +1710,7 @@ static void __sched rtlock_slowlock_locked(struct rt_mutex_base *lock)
 		if (try_to_take_rt_mutex(lock, current, &waiter))
 			break;
 
-		if (&waiter == rt_mutex_top_waiter(lock))
-			owner = rt_mutex_owner(lock);
-		else
-			owner = NULL;
+		owner = rt_mutex_owner(lock);
 		raw_spin_unlock_irq(&lock->wait_lock);
 
 		if (!owner || !rtmutex_spin_on_owner(lock, &waiter, owner))