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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Apr 2, 2025 at 2:35 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Apr 2, 2025 at 2:02 AM Vlastimil Babka <vbabka@suse.cz> wrote:
> >
> > On 4/2/25 09:30, Sebastian Andrzej Siewior wrote:
> > > On 2025-03-31 17:51:34 [-0700], Alexei Starovoitov wrote:
> > >> From: Alexei Starovoitov <ast@kernel.org>
> > >>
> > >> Partially revert commit 0aaddfb06882 ("locking/local_lock: Introduce localtry_lock_t").
> > >> Remove localtry_*() helpers, since localtry_lock() name might
> > >> be misinterpreted as "try lock".
> > >
> > > So we back to what you suggested initially. I was more a fan of
> > > explicitly naming things but if this is misleading so be it. So
> > >
> > > Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > >
> > > While at it, could you look at the hunk below and check if it worth it?
> > > The struct duplication and hoping that the first part remains the same,
> > > is hoping. This still relies that the first part remains the same but…
> >
> > I've updated your fixups to v2
> > https://lore.kernel.org/all/20250401205245.70838-1-alexei.starovoitov@gmail.com/
>
> Sebastian, Vlastimil,
> Thanks for the fixups. Folded.

I was about to send v3, but decided to play with
my stress test a bit more and it caught this race:

[   18.114617] DEBUG_LOCKS_WARN_ON(l->owner)
[   18.114717] WARNING: CPU: 0 PID: 2159 at
include/linux/local_lock_internal.h:46 try_charge_memcg+0x73b/0x7b0
[   18.114733] RIP: 0010:try_charge_memcg+0x73b/0x7b0
[   18.114769]  <NMI>
[   18.114774]  __memcg_kmem_charge_page+0xbe/0x390
[   18.114778]  try_alloc_pages_noprof+0x11e/0x280
[   18.114782]  nmi_callback+0x55/0xc0 [bpf_testmod]
...
[   18.114820]  end_repeat_nmi+0xf/0x18
[   18.114822] RIP: 0010:refill_stock+0xef/0x1e0
[   18.114832]  ? refill_stock+0xef/0x1e0
[   18.114834]  </NMI>
[   18.114834]  <TASK>
[   18.114835]  obj_cgroup_uncharge_pages+0x44/0x170
[   18.114837]  __memcg_kmem_uncharge_page+0x52/0x180
[   18.114839]  __free_frozen_pages+0xce/0x6c0

The same issue is present in the __localtry_unlock*()...
these two should be done in the opposite order:
    WRITE_ONCE(lt->acquired, 0);
    local_lock_release(&lt->llock);

Though IRQs are still disabled at this point
the local_trylock() from NMI will succeed and
local_lock_acquire(l); will warn.

The following fix will be included in v3:

diff --git a/include/linux/local_lock_internal.h
b/include/linux/local_lock_internal.h
index e41ca62fadea..bf2bf40d7b18 100644
--- a/include/linux/local_lock_internal.h
+++ b/include/linux/local_lock_internal.h
@@ -169,13 +169,13 @@ do {
                 \
                                                                        \
                l = (local_lock_t *)this_cpu_ptr(lock);                 \
                tl = (local_trylock_t *)l;                              \
+               local_lock_release(l);                                  \
                _Generic((lock),                                        \
                        local_trylock_t *: ({                           \
                                lockdep_assert(tl->acquired == 1);      \
                                WRITE_ONCE(tl->acquired, 0);            \
                        }),                                             \
                        default:(void)0);                               \
-               local_lock_release(l);                                  \
        } while (0)

After that the stress tester runs cleanly.