Re: [PATCH RFC 0/2] mm, swap: fix swapin race that causes inaccurate memcg accounting

[Kairui Song <ryncsn@gmail.com>]

On Mon, Apr 13, 2026 at 3:54 PM YoungJun Park <youngjun.park@lge.com> wrote:
>
> On Tue, Apr 07, 2026 at 10:55:41PM +0800, Kairui Song via B4 Relay wrote:
> > While doing code inspection, I noticed there is a long-existing issue
> > THP swapin may got charged into the wrong memcg since commit
> > 242d12c981745 ("mm: support large folios swap-in for sync io devices").
> > And a recent fix made it a bit worse.
> >

...

> > SYNCHRONOUS_IO fix seems also good, but it changes the current fallback
> > logic. Instead of fallback to next order it will fallback to order 0
> > directly. That should be fine though. This issue can be fixed / cleaned
> > up in a better way with swap table P4 as demostrated previously by
> > allocating the folio in swap cache directly with proper fallback and a
> > more compat loop for error handling:
> >
> > https://lore.kernel.org/linux-mm/20260220-swap-table-p4-v1-4-104795d19815@tencent.com/
>
> Hello Kairui,
>
> Nice catch!
>
> I have reviewed the proposed patches, and LGTM :D
> (For 1/2, flattening the if-statement depth slightly could help readability.
> However, since this is planned to be refactored as part of the P4 swap table work,
> I think it is fine as is.)

Hi YoungJun

>
> I mostly agree with your rationale.
>
> > memcg0 is not completely irrelevant as it's true that it is now
> > memcg1 faulting this folio. Shmem may have similar issue.
>
> That said, I would like to leave one small comment.
>
> My understanding is that if we account based on the folio that was
> allocated while running in memcg0 (on CPU 0), then having
> set_pte_at() install it with memcg0 already charged may still be
> considered acceptable from a acceptable coarse-grained synchronization perspective.
> (cuz folio is alloced at the time of "memcg 1 epoch")

Right... which is also why I sent it as an RFC, I wasn't completely
sure that if I missed anything. Charging into memcg0 is not really
that wrong, so this might be a negligible problem.

>
> Let's think of the situation below
>
>   CPU 0 (memcg0)                 CPU 1
>   ---------------------------    -----------------------------
>   charge folio to memcg0
>   allocate / prepare folio
>                                    task migrates to memcg1
>   ...
>   set_pte_at() installs PTE
>   (folio is already charged to memcg0)
>
> In this flow, the charge follows the allocation context (memcg0),
> even though the actual PTE installation happens after migration
> to memcg1.
>
> I understand that we cannot strictly guarantee correctness without
> fully synchronized migration, so this region inherently has some
> ambiguity. In that sense, the patch is addressing a corner of that
> problem space.
>
> But, I largely agree with your argument (the rationale is sound,
> and the change is not intrusive).
>
> I would have no further concerns if the following hold:
>
> - There is a tangible benefit to modifying this patch.

I can't really say that. The effect might be hardly observable, the
time window is really short and a few pages of inaccuracy (and in this
case, it's not completely inaccurate, just ambiguous) of the memcg
counter is hard to detect too.

> - There is no meaningful behavioral difference between charging
>   earlier (current behavior) and charging later (proposed change),
>   (e.g especially when memcg limits are hit.)

This part should be fine. Charge after swap cache might help to avoid thrashing.

> If those assumptions are correct, I am fully on board.

Thanks! It seems the benefit of this RFC is indeed trivial. I also ran
some performance tests later and didn't observe anything meaningful so
far.

Maybe we can then just go with the swap table p4 series directly, I
might overthinked about the potential issues, it would be solved
cleaner if we skip this here.