Re: Linux 6.15-rc3

[Vlastimil Babka <vbabka@suse.cz>]

On 4/23/25 02:54, Michael Larabel wrote:
> On 4/22/25 7:12 PM, Shakeel Butt wrote:
>> Ccing Michael
>>
>> On Tue, Apr 22, 2025 at 04:37:59PM -0700, Alexei Starovoitov wrote:
>>> On Tue, Apr 22, 2025 at 4:01 PM Dave Airlie <airlied@gmail.com> wrote:
>>>>> Alexei Starovoitov (2):
>>>>>        locking/local_lock, mm: replace localtry_ helpers with
>>>>> local_trylock_t type
>>>> This seems to have upset some phoronix nginx workload
>>>> https://www.phoronix.com/review/linux-615-nginx-regression/2
>>> 3x regression? wow.
>>> Thanks for heads up.
>>> I'm staring at the patch and don't see it.
>>> Adding more experts.
>> Hi Michael,
>>
>> Can you please share a bit more on your nginx workload and how can we
>> reproduce locally? In the mean time, I can try netperf locally to
>> reproduce.
>>
>> I do have some followup optimizations [1, 2] which hopefully are aimed
>> for next release but we can try those as well.
>>
>> [1] https://lkml.kernel.org/r/20250416180229.2902751-1-shakeel.butt@linux.dev
>> [2] https://lkml.kernel.org/r/20250410025752.92159-1-shakeel.butt@linux.dev
>>
>> thanks,
>> Shakeel
> 
> The Nginx test case is a fairly stock Nginx build measuring HTTPS 
> throughput for serving some static web page with Wrk used as the 
> stressor, all on the same host for stressing just the local host. All of 
> the assets and execution scripts used for that Nginx test in raw form 
> are here - 
> https://openbenchmarking.org/innhold/c7b555063f5732b4f1bb08444e258984d1dbb94b 
> Let me know if any problems reproducing, etc.
> 
> Thanks, I can try out those patches tomorrow. At the moment on that same 
> server currently running through some of the other workloads I also 
> found regressed on Linux 6.15 GIt to see if attributed to same commit or 
> not.

Hi, please try the following patch. I also realized I left you out of my previous
replies leading to it, due to replying to an earlier mail of the thread:
https://lore.kernel.org/all/0981c1fe-05d2-4bab-a0a4-6dc5666d98d7@suse.cz/

Thanks,
Vlastimil

----8<----
From: Vlastimil Babka <vbabka@suse.cz>
Date: Wed, 23 Apr 2025 10:21:29 +0200
Subject: [PATCH] locking/local_lock: fix _Generic() matching of
 local_trylock_t

Michael Larabel reported [1] a nginx performance regression in v6.15-rc3 and
bisected it to commit 51339d99c013 ("locking/local_lock, mm: replace
localtry_ helpers with local_trylock_t type")

The problem is the _Generic() usage with a default association that
masks the fact that "local_trylock_t *" association is not being
selected as expected. Replacing the default with the only other expected
type "local_lock_t *" reveals the underlying problem:

./include/linux/local_lock_internal.h:174:26: error: ‘_Generic’ selector of
type ‘__seg_gs local_lock_t *’ is not compatible with any association

The local_locki's are part of __percpu structures and thus the __percpu
attribute is needed to associate the type properly. Add the attribute
and keep the default replaced to turn any further mismatches into
compile errors.

The failure to recognize local_try_lock_t in __local_lock_release()
means that a local_trylock[_irqsave]() operation will set tl->acquired
to 1 (there's no _Generic() part in the trylock code), but then
local_unlock[_irqrestore]() will not set tl->acquired back to 0, so
further trylock operations will always fail on the same cpu+lock, while
non-trylock operations continue to work - a lockdep_assert() is also
not being executed in the _Generic() part of local_lock() code.

This means consume_stock() and refill_stock() operations will fail
deterministically, resulting in taking the slow paths and worse
performance.

Fixes: 51339d99c013 ("locking/local_lock, mm: replace localtry_ helpers with local_trylock_t type")
Reported-by: Michael Larabel <Michael@phoronix.com>
Closes: https://www.phoronix.com/review/linux-615-nginx-regression/2 [1]
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/local_lock_internal.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
index bf2bf40d7b18..8d5ac16a9b17 100644
--- a/include/linux/local_lock_internal.h
+++ b/include/linux/local_lock_internal.h
@@ -102,11 +102,11 @@ do {								\
 		l = (local_lock_t *)this_cpu_ptr(lock);			\
 		tl = (local_trylock_t *)l;				\
 		_Generic((lock),					\
-			local_trylock_t *: ({				\
+			__percpu local_trylock_t *: ({			\
 				lockdep_assert(tl->acquired == 0);	\
 				WRITE_ONCE(tl->acquired, 1);		\
 			}),						\
-			default:(void)0);				\
+			__percpu local_lock_t *: (void)0);		\
 		local_lock_acquire(l);					\
 	} while (0)
 
@@ -171,11 +171,11 @@ do {								\
 		tl = (local_trylock_t *)l;				\
 		local_lock_release(l);					\
 		_Generic((lock),					\
-			local_trylock_t *: ({				\
+			__percpu local_trylock_t *: ({			\
 				lockdep_assert(tl->acquired == 1);	\
 				WRITE_ONCE(tl->acquired, 0);		\
 			}),						\
-			default:(void)0);				\
+			__percpu local_lock_t *: (void)0);		\
 	} while (0)
 
 #define __local_unlock(lock)					\
-- 
2.49.0