Re: Linux 6.15-rc3

[Michael Larabel <Michael@phoronix.com>]

On 4/23/25 3:42 AM, Vlastimil Babka wrote:
> 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


Thanks, Vlastimil. I tested your later patch @ 
https://lore.kernel.org/all/d1c0f057-63c8-4be5-9638-d1a67d9b9e15@suse.cz/#t

I can confirm that latest Git + that patch does indeed correct the 
regression affecting Nginx. All the Nginx runs I have carried out this 
morning are back aligned with the 6.14 and the results prior to the 
bisected commit.

Currently repeating tests for other regressed workloads to confirm there 
but at least for the Nginx case is resolved by that patch.

Michael


>
> ----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)					\