Re: [PATCH v3 10/11] mm, swap: defer memcg_table allocation for physical swap clusters

On Thu, Aug 6, 2026 at 12:19 PM <[email protected]> wrote:

Hmm seems like this is not sent to the list. I'll just cc the response
to my original sent list...

>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Deferring `memcg_table` allocation to `alloc_swap_scan_cluster` under `local_lock` using `GFP_ATOMIC` causes an infinite loop under memory pressure and violates allocation constraints.
> --
>
> > +#ifdef CONFIG_MEMCG
> > +             /*
> > +              * Lazy-allocate memcg_table on the first direct-use slot of a
> > +              * physical cluster.
> > +              */
> > +             if (IS_ENABLED(CONFIG_VSWAP) && folio &&
> > +                 !folio_test_swapcache(folio) && !mem_cgroup_disabled() &&
> > +                 !ci->memcg_table) {
> > +                     ci->memcg_table = kzalloc_obj(*ci->memcg_table,
> > +                                                   GFP_ATOMIC | __GFP_NOWARN);
> > +                     if (!ci->memcg_table)
> > +                             goto out;
> > +             }
> > +#endif
>
> [Severity: Critical]
> Does this error path create an infinite loop under memory pressure?
>
> If the GFP_ATOMIC allocation fails, we jump to the out label, which calls
> relocate_cluster() and puts the unmodified cluster back on the list,
> returning 0.
>
> When called via this path:
>
> cluster_alloc_swap_entry()
>   alloc_swap_scan_list(..., scan_all = true)
>     do {
>         ci = isolate_lock_cluster(si, list);
>         found = alloc_swap_scan_cluster(si, ci, ...);
>         if (found)
>             break;
>     } while (scan_all);
>
> Won't alloc_swap_scan_list() continuously dequeue the exact same cluster
> and fail the allocation indefinitely, since the cluster is never consumed
> or discarded?

Oh nice catch! It's a a bit of a rare construction. You need:

1. Vswap enabled.

2. The physical swap clusters start out backing vswap entries.

3. Then we use these same cluster to directly map to PTEs, bypassing
vswap. This usually means vswap allocation fails (rare), or is disabled
during runtime (rare).

4. The deferred memcg_table (1KB per cluster IIRC) allocation must fail.

Technically possible, but does not seem very likely, so I'll just do
the minimum to not infinite-loop here: i.e if memcg_table allocation
fails, just propagate that information up the call stack and
short-circuit the list scan.

We can add complexity (dropping the lock and retry allocation)
if the rate of allocation failure is high enough in this case (which I
doubt).

>
> Additionally, alloc_swap_scan_cluster() runs with preemption disabled under
> a local_lock. Does using GFP_ATOMIC here risk lock recursion? GFP_ATOMIC
> includes __GFP_KSWAPD_RECLAIM, which wakes kswapd and can take scheduler
> locks while preemption is disabled.

I don't think this is correct.