Re: [PATCH] locking/local_lock, mm: Replace localtry_ helpers with local_trylock_t type

[Alexei Starovoitov]

On Thu, Apr 3, 2025 at 2:09 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
>
> On 4/2/25 23:35, Alexei Starovoitov wrote:
> > On Wed, Apr 2, 2025 at 2:02 AM Vlastimil Babka <vbabka@xxxxxxx> 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.