| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <27d55aab-c2d8-4edf-bab5-91a04b8383c5@suse.cz>
Date: Wed, 23 Apr 2025 10:42:03 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Michael Larabel <Michael@...ronix.com>,
Shakeel Butt <shakeel.butt@...ux.dev>
Cc: Dave Airlie <airlied@...il.com>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
Sebastian Sewior <bigeasy@...utronix.de>,
Andrew Morton <akpm@...ux-foundation.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Alexei Starovoitov <ast@...nel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: Linux 6.15-rc3
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@...il.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@...e.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@...ronix.com>
Closes: https://www.phoronix.com/review/linux-615-nginx-regression/2 [1]
Signed-off-by: Vlastimil Babka <vbabka@...e.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
Powered by blists - more mailing lists