On 23/10/2024 19:02, Yosry Ahmed wrote:
> [..]
>>>> I suspect the regression occurs because you're running an edge case
>>>> where the memory cgroup stays nearly full most of the time (this isn't
>>>> an inherent issue with large folio swap-in). As a result, swapping in
>>>> mTHP quickly triggers a memcg overflow, causing a swap-out. The
>>>> next swap-in then recreates the overflow, leading to a repeating
>>>> cycle.
>>>>
>>>
>>> Yes, agreed! Looking at the swap counters, I think this is what is going
>>> on as well.
>>>
>>>> We need a way to stop the cup from repeatedly filling to the brim and
>>>> overflowing. While not a definitive fix, the following change might help
>>>> improve the situation:
>>>>
>>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>>>
>>>> index 17af08367c68..f2fa0eeb2d9a 100644
>>>> --- a/mm/memcontrol.c
>>>> +++ b/mm/memcontrol.c
>>>>
>>>> @@ -4559,7 +4559,10 @@ int mem_cgroup_swapin_charge_folio(struct folio
>>>> *folio, struct mm_struct *mm,
>>>> memcg = get_mem_cgroup_from_mm(mm);
>>>> rcu_read_unlock();
>>>>
>>>> - ret = charge_memcg(folio, memcg, gfp);
>>>> + if (folio_test_large(folio) && mem_cgroup_margin(memcg) <
>>>> MEMCG_CHARGE_BATCH)
>>>> + ret = -ENOMEM;
>>>> + else
>>>> + ret = charge_memcg(folio, memcg, gfp);
>>>>
>>>> css_put(&memcg->css);
>>>> return ret;
>>>> }
>>>>
>>>
>>> The diff makes sense to me. Let me test later today and get back to you.
>>>
>>> Thanks!
>>>
>>>> Please confirm if it makes the kernel build with memcg limitation
>>>> faster. If so, let's
>>>> work together to figure out an official patch :-) The above code hasn't consider
>>>> the parent memcg's overflow, so not an ideal fix.
>>>>
>>
>> Thanks Barry, I think this fixes the regression, and even gives an improvement!
>> I think the below might be better to do:
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index c098fd7f5c5e..0a1ec55cc079 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4550,7 +4550,11 @@ int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm,
>> memcg = get_mem_cgroup_from_mm(mm);
>> rcu_read_unlock();
>>
>> - ret = charge_memcg(folio, memcg, gfp);
>> + if (folio_test_large(folio) &&
>> + mem_cgroup_margin(memcg) < max(MEMCG_CHARGE_BATCH, folio_nr_pages(folio)))
>> + ret = -ENOMEM;
>> + else
>> + ret = charge_memcg(folio, memcg, gfp);
>>
>> css_put(&memcg->css);
>> return ret;
>>
>>
>> AMD 16K+32K THP=always
>> metric mm-unstable mm-unstable + large folio zswapin series mm-unstable + large folio zswapin + no swap thrashing fix
>> real 1m23.038s 1m23.050s 1m22.704s
>> user 53m57.210s 53m53.437s 53m52.577s
>> sys 7m24.592s 7m48.843s 7m22.519s
>> zswpin 612070 999244 815934
>> zswpout 2226403 2347979 2054980
>> pgfault 20667366 20481728 20478690
>> pgmajfault 385887 269117 309702
>>
>> AMD 16K+32K+64K THP=always
>> metric mm-unstable mm-unstable + large folio zswapin series mm-unstable + large folio zswapin + no swap thrashing fix
>> real 1m22.975s 1m23.266s 1m22.549s
>> user 53m51.302s 53m51.069s 53m46.471s
>> sys 7m40.168s 7m57.104s 7m25.012s
>> zswpin 676492 1258573 1225703
>> zswpout 2449839 2714767 2899178
>> pgfault 17540746 17296555 17234663
>> pgmajfault 429629 307495 287859
>>
>
> Thanks Usama and Barry for looking into this. It seems like this would
> fix a regression with large folio swapin regardless of zswap. Can the
> same result be reproduced on zram without this series?
Yes, its a regression in large folio swapin support regardless of zswap/zram.
Need to do 3 tests, one with probably the below diff to remove large folio support,
one with current upstream and one with upstream + swap thrashing fix.
We only use zswap and dont have a zram setup (and I am a bit lazy to create one :)).
Any zram volunteers to try this?
diff --git a/mm/memory.c b/mm/memory.c
index fecdd044bc0b..62f6b087beb3 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4124,6 +4124,8 @@ static struct folio *alloc_swap_folio(struct vm_fault *vmf)
gfp_t gfp;
int order;
+ goto fallback;
+
/*
* If uffd is active for the vma we need per-page fault fidelity to
* maintain the uffd semantics.
