public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: "Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: <willy@infradead.org>, <linux-kernel@vger.kernel.org>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"intel-xe@lists.freedesktop.org" <intel-xe@lists.freedesktop.org>,
	"Kurmi, Suresh Kumar" <suresh.kumar.kurmi@intel.com>,
	"Saarinen, Jani" <jani.saarinen@intel.com>,
	<ravitejax.veesam@intel.com>
Subject: Re: Regression on linux-next (next-20260324 )
Date: Tue, 21 Apr 2026 20:01:43 +0530	[thread overview]
Message-ID: <d5b882b8-19a9-4e83-b6a0-ecfad3e61522@intel.com> (raw)
In-Reply-To: <20260420130318.GD3102924@noisy.programming.kicks-ass.net>

Hello Peter,

On 4/20/2026 6:33 PM, Peter Zijlstra wrote:
> On Mon, Mar 30, 2026 at 09:50:37PM +0200, Peter Zijlstra wrote:
>> On Mon, Mar 30, 2026 at 01:56:33PM +0530, Borah, Chaitanya Kumar wrote:
>>>> diff --git a/kernel/locking/ww_mutex.h b/kernel/locking/ww_mutex.h
>>>> index b1834ab7e782..bb8b410779d4 100644
>>>> --- a/kernel/locking/ww_mutex.h
>>>> +++ b/kernel/locking/ww_mutex.h
>>>> @@ -42,7 +42,7 @@ __ww_waiter_last(struct mutex *lock)
>>>>    	struct mutex_waiter *w = lock->first_waiter;
>>>>    	if (w)
>>>> -		w = list_prev_entry(w, list);
>>>> +		w = __ww_waiter_prev(lock, w);
>>>>    	return w;
>>>>    }
>>> Thank you for the response, Peter. Unfortunately, the issue is still seen
>>> with this change.
>>
>> Bah, indeed. Looking at this after the weekend I see that it's actually
>> wrong.
>>
>> But I haven't yet had a new idea. I don't suppose there is a relatively
>> easy way to reproduce this issue outside of your CI robot?
>>
>> My current working thesis is that since this is graphics, this is
>> ww_mutex related. I'll go over this code once more...
> 
> Since you've not provided a reproducer, can I ask you to try the below?
> 
> ---
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index 186b463fe326..a93e57fc53b1 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -229,10 +229,8 @@ __mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter)
>   		__mutex_clear_flag(lock, MUTEX_FLAGS);
>   		lock->first_waiter = NULL;
>   	} else {
> -		if (lock->first_waiter == waiter) {
> -			lock->first_waiter = list_first_entry(&waiter->list,
> -							      struct mutex_waiter, list);
> -		}
> +		if (lock->first_waiter == waiter)
> +			lock->first_waiter = list_next_entry(waiter, list);
>   		list_del(&waiter->list);
>   	}
>   
> diff --git a/kernel/locking/ww_mutex.h b/kernel/locking/ww_mutex.h
> index 016f0db892a5..875b303511b3 100644
> --- a/kernel/locking/ww_mutex.h
> +++ b/kernel/locking/ww_mutex.h
> @@ -6,6 +6,32 @@
>   #define MUTEX_WAITER	mutex_waiter
>   #define WAIT_LOCK	wait_lock
>   
> +/*
> + *                +-------+
> + *                |   3   | <+
> + *                +-------+  |
> + *                    ^      |
> + *                    |      |
> + *                    v      |
> + *  +-------+     +-------+  |
> + *  | first | --> |   1   |  |
> + *  +-------+     +-------+  |
> + *                    ^      |
> + *                    |      |
> + *                    v      |
> + *                +-------+  |
> + *                |   2   | <+
> + *                +-------+
> + */
> +
> +/*
> + * Specifically:
> + *
> + *   for (cur = __ww_waiter_first(); cur; cur = __ww_waiter_next())
> + *     ...
> + *
> + * should iterate like: 1 2 3
> + */
>   static inline struct mutex_waiter *
>   __ww_waiter_first(struct mutex *lock)
>   	__must_hold(&lock->wait_lock)
> @@ -18,23 +44,21 @@ __ww_waiter_next(struct mutex *lock, struct mutex_waiter *w)
>   	__must_hold(&lock->wait_lock)
>   {
>   	w = list_next_entry(w, list);
> -	if (lock->first_waiter == w)
> -		return NULL;
> -
> -	return w;
> -}
> -
> -static inline struct mutex_waiter *
> -__ww_waiter_prev(struct mutex *lock, struct mutex_waiter *w)
> -	__must_hold(&lock->wait_lock)
> -{
> -	w = list_prev_entry(w, list);
> -	if (lock->first_waiter == w)
> +	/* We've already seen first, terminate */
> +	if (w == __ww_waiter_first(lock))
>   		return NULL;
>   
>   	return w;
>   }
>   
> +/*
> + * Specifically:
> + *
> + *   for (cur = __ww_waiter_last(); cur; cur = __ww_waiter_prev())
> + *     ...
> + *
> + * should iterate like: 3 2 1
> + */
>   static inline struct mutex_waiter *
>   __ww_waiter_last(struct mutex *lock)
>   	__must_hold(&lock->wait_lock)
> @@ -46,6 +70,18 @@ __ww_waiter_last(struct mutex *lock)
>   	return w;
>   }
>   
> +static inline struct mutex_waiter *
> +__ww_waiter_prev(struct mutex *lock, struct mutex_waiter *w)
> +	__must_hold(&lock->wait_lock)
> +{
> +	w = list_prev_entry(w, list);
> +	/* We've already seen last, terminate */
> +	if (w == __ww_waiter_last(lock))
> +		return NULL;
> +
> +	return w;
> +}
> +
>   static inline void
>   __ww_waiter_add(struct mutex *lock, struct mutex_waiter *waiter, struct mutex_waiter *pos)
>   	__must_hold(&lock->wait_lock)

Thank you for the patch.
This seems to fix the issue on our CI machine. The diff turned out to be 
slightly different, pasting it here just in case.

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 186b463fe326..a93e57fc53b1 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -229,10 +229,8 @@ __mutex_remove_waiter(struct mutex *lock, struct 
mutex_waiter *waiter)
                 __mutex_clear_flag(lock, MUTEX_FLAGS);
                 lock->first_waiter = NULL;
         } else {
-               if (lock->first_waiter == waiter) {
-                       lock->first_waiter = list_first_entry(&waiter->list,
-                                                             struct 
mutex_waiter, list);
-               }
+               if (lock->first_waiter == waiter)
+                       lock->first_waiter = list_next_entry(waiter, list);
                 list_del(&waiter->list);
         }

diff --git a/kernel/locking/ww_mutex.h b/kernel/locking/ww_mutex.h
index 016f0db892a5..875b303511b3 100644
--- a/kernel/locking/ww_mutex.h
+++ b/kernel/locking/ww_mutex.h
@@ -6,6 +6,32 @@
#define MUTEX_WAITER   mutex_waiter
#define WAIT_LOCK      wait_lock

+/*
+ *                +-------+
+ *                |   3   | <+
+ *                +-------+  |
+ *                    ^      |
+ *                    |      |
+ *                    v      |
+ *  +-------+     +-------+  |
+ *  | first | --> |   1   |  |
+ *  +-------+     +-------+  |
+ *                    ^      |
+ *                    |      |
+ *                    v      |
+ *                +-------+  |
+ *                |   2   | <+
+ *                +-------+
+ */
+
+/*
+ * Specifically:
+ *
+ *   for (cur = __ww_waiter_first(); cur; cur = __ww_waiter_next())
+ *     ...
+ *
+ * should iterate like: 1 2 3
+ */
static inline struct mutex_waiter *
__ww_waiter_first(struct mutex *lock)
         __must_hold(&lock->wait_lock)
@@ -18,31 +44,41 @@ __ww_waiter_next(struct mutex *lock, struct 
mutex_waiter *w)
         __must_hold(&lock->wait_lock)
{
         w = list_next_entry(w, list);
-       if (lock->first_waiter == w)
+       /* We've already seen first, terminate */
+       if (w == __ww_waiter_first(lock))
                 return NULL;

         return w;
}

+/*
+ * Specifically:
+ *
+ *   for (cur = __ww_waiter_last(); cur; cur = __ww_waiter_prev())
+ *     ...
+ *
+ * should iterate like: 3 2 1
+ */
static inline struct mutex_waiter *
-__ww_waiter_prev(struct mutex *lock, struct mutex_waiter *w)
+__ww_waiter_last(struct mutex *lock)
         __must_hold(&lock->wait_lock)
{
-       w = list_prev_entry(w, list);
-       if (lock->first_waiter == w)
-               return NULL;
+       struct mutex_waiter *w = lock->first_waiter;

+       if (w)
+               w = list_prev_entry(w, list);
         return w;
}

static inline struct mutex_waiter *
-__ww_waiter_last(struct mutex *lock)
+__ww_waiter_prev(struct mutex *lock, struct mutex_waiter *w)
         __must_hold(&lock->wait_lock)
{
-       struct mutex_waiter *w = lock->first_waiter;
+       w = list_prev_entry(w, list);
+       /* We've already seen last, terminate */
+       if (w == __ww_waiter_last(lock))
+               return NULL;

-       if (w)
-               w = list_prev_entry(w, list);
         return w;
}

==
Chaitanya



      parent reply	other threads:[~2026-04-21 14:31 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-27 13:39 Regression on linux-next (next-20260324 ) Borah, Chaitanya Kumar
2026-03-27 16:31 ` Peter Zijlstra
2026-03-27 16:43   ` Peter Zijlstra
2026-03-30  8:26     ` Borah, Chaitanya Kumar
2026-03-30 19:50       ` Peter Zijlstra
2026-04-20 13:03         ` Peter Zijlstra
2026-04-21  6:45           ` John Stultz
2026-04-21 10:15             ` Peter Zijlstra
2026-04-21 12:54               ` K Prateek Nayak
2026-04-21 14:37                 ` Peter Zijlstra
2026-04-21 14:45                   ` Matthew Wilcox
2026-04-21 15:03                     ` Peter Zijlstra
2026-04-21 15:48                   ` K Prateek Nayak
2026-04-21 17:29                     ` John Stultz
2026-04-21 14:31           ` Borah, Chaitanya Kumar [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d5b882b8-19a9-4e83-b6a0-ecfad3e61522@intel.com \
    --to=chaitanya.kumar.borah@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=intel-xe@lists.freedesktop.org \
    --cc=jani.saarinen@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=ravitejax.veesam@intel.com \
    --cc=suresh.kumar.kurmi@intel.com \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox