linux-kernel - Re: [PATCH] locking/local_lock, mm: Replace localtry_ helpers with local_trylock

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAADnVQJjy6fFGpPCm_bo-MmpgP136jxtn6De-AiemL28de91MA@mail.gmail.com>
Date: Thu, 3 Apr 2025 07:44:07 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>, Linus Torvalds <torvalds@...ux-foundation.org>, 
	bpf <bpf@...r.kernel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...nel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Peter Zijlstra <peterz@...radead.org>, 
	Steven Rostedt <rostedt@...dmis.org>, Shakeel Butt <shakeel.butt@...ux.dev>, 
	Michal Hocko <mhocko@...e.com>, linux-mm <linux-mm@...ck.org>, 
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] locking/local_lock, mm: Replace localtry_ helpers with
 local_trylock_t type

On Thu, Apr 3, 2025 at 2:09 AM Vlastimil Babka <vbabka@...e.cz> wrote:
>
> On 4/2/25 23:35, Alexei Starovoitov wrote:
> > On Wed, Apr 2, 2025 at 2:02 AM Vlastimil Babka <vbabka@...e.cz> wrote:
> >
> > This is because the macro specifies the type:
> > DEFINE_GUARD(local_lock, local_lock_t __percpu*,
> >
> > and that type is used to define two static inline functions
> > with that type,
> > so by the time our __local_lock_acquire() macro is used
> > it sees 'local_lock_t *' and not the actual type of memcg.stock_lock.
>
> Hm but I didn't even try to instantiate any guard. In fact the compilation
> didn't even error on compiling my slub.o but earlier in compiling
> arch/x86/kernel/asm-offsets.c

The compiler will error compiling a random first file in your build process
that happens to include local_lock.h.
In your case it was asm-offsets.c.
Try make mm/ and it will be another file.
It doesn't matter that nothing is using guard(local_lock).
The static inline functions are there in that compilation unit and
they have to go through the compiler front-end to be discarded
as unused by the middle end later.

> I think the problem is rather that the guard creates static inline functions
> and _Generic() only works via macros as you pointed out in the reply to Andrew?
>
> I guess it's solvable if we care in the future, but it means more code
> duplication - move the _Generic() dispatch outside the whole implementation
> to choose between two variants, have guards use use the specific variant
> directly without _Generic()?

Unlikely. _Generic() works only when the original expression
is preserved all the way to _Generic() statement.

> Or maybe there's a simpler way I'm just not familiar with both the guards
> and _Generic() enough.

There are options.
Here is one:
diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h
index 1a0bc35839e3..e053e187d99d 100644
--- a/include/linux/local_lock.h
+++ b/include/linux/local_lock.h
@@ -124,6 +124,9 @@
 DEFINE_GUARD(local_lock, local_lock_t __percpu*,
             local_lock(_T),
             local_unlock(_T))
+DEFINE_GUARD(local_lock_for_trylock, local_trylock_t __percpu*,
+            local_lock(_T),
+            local_unlock(_T))

and it can be used as:
guard(local_lock_for_trylock)(&lock)

Naming is hard, of course.

tbh I'm not a fan of guard()-s because I see it's being used
in cases when open coded lock/unlock would be more readable.
They're useful when there are plenty of cleanup code and goto-s,
but not universally.

In case of local_trylock() the guard is likely unusable.
The pattern, so far, is something like:
if (condition)
   local_trylock_...(&lock);
else
   local_lock_...(&lock);

so guard()-s don't fit.