Re: [PATCH v2 3/3] memcg: no irq disable for memcg stock lock

[Alexei Starovoitov]

On Thu, May 1, 2025 at 5:18 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:
>
> There is no need to disable irqs to use memcg per-cpu stock, so let's
> just not do that. One consequence of this change is if the kernel while
> in task context has the memcg stock lock and that cpu got interrupted.
> The memcg charges on that cpu in the irq context will take the slow path
> of memcg charging. However that should be super rare and should be fine
> in general.
>
> Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
> ---
>  mm/memcontrol.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index cd81c70d144b..f8b9c7aa6771 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1858,7 +1858,6 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages,
>  {
>         struct memcg_stock_pcp *stock;
>         uint8_t stock_pages;
> -       unsigned long flags;
>         bool ret = false;
>         int i;
>
> @@ -1866,8 +1865,8 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages,
>                 return ret;
>
>         if (gfpflags_allow_spinning(gfp_mask))
> -               local_lock_irqsave(&memcg_stock.lock, flags);
> -       else if (!local_trylock_irqsave(&memcg_stock.lock, flags))
> +               local_lock(&memcg_stock.lock);
> +       else if (!local_trylock(&memcg_stock.lock))
>                 return ret;

I don't think it works.
When there is a normal irq and something doing regular GFP_NOWAIT
allocation gfpflags_allow_spinning() will be true and
local_lock() will reenter and complain that lock->acquired is
already set... but only with lockdep on.