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

[Vlastimil Babka <vbabka@suse.cz>]

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

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()?

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

> Your macro can be hacked with addition of:
> local_lock_t *l = NULL;
> ...
> l = (void *)this_cpu_ptr(lock);
> ...
> tl = (void *)this_cpu_ptr(lock);
> ...
> DEFINE_GUARD(local_lock, void __percpu*,
> 
> then
> guard(local_lock)(&memcg_stock.stock_lock);
> 
> will compile without warnings with both
> typeof(stock_lock) = local_lock_t and local_trylock_t,
> 
> but the generated code will take default:(void)0) path
> and will pass NULL into local_lock_acquire(NULL);
> 
> In other words guard(local_lock) can only support one
> specific type. It cannot be made polymorphic with _Generic() trick.
> This is an unfortunate tradeoff with this approach.
> Thankfully there are no users of it in the tree:
> git grep 'guard(local'|wc -l
> 0
> 
> so I think it's ok that guard(local_lock) can only be used with local_lock_t.