Re: [RFC 0/4] mm: zswap: add support for zswapin of large folios

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 24, 2024 at 8:47 AM Usama Arif <usamaarif642@xxxxxxxxx> wrote:
>
>
>
> On 23/10/2024 19:52, Barry Song wrote:
> > On Thu, Oct 24, 2024 at 7:31 AM Usama Arif <usamaarif642@xxxxxxxxx> wrote:
> >>
> >>
> >>
> >> 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?
> >
> > Hi Usama,
> >
> > I tried a quick experiment:
> >
> > echo 1 > /sys/module/zswap/parameters/enabled
> > echo 0 > /sys/module/zswap/parameters/enabled
> >
> > This was to test the zRAM scenario. Enabling zswap even
> > once disables mTHP swap-in. :)
> >
> > I noticed a similar regression with zRAM alone, but the change resolved
> > the issue and even sped up the kernel build compared to the setup without
> > mTHP swap-in.
>
> Thanks for trying, this is amazing!
> >
> > However, I’m still working on a proper patch to address this. The current
> > approach:
> >
> > mem_cgroup_margin(memcg) < max(MEMCG_CHARGE_BATCH, folio_nr_pages(folio))
> >
> > isn’t sufficient, as it doesn’t cover cases where group A contains group B, and
> > we’re operating within group B. The problem occurs not at the boundary of
> > group B but at the boundary of group A.
>
> I am not sure I completely followed this. As MEMCG_CHARGE_BATCH=64, if we are
> trying to swapin a 16kB page, we basically check if atleast 64/4 = 16 folios can be
> charged to cgroup, which is reasonable. If we try to swapin a 1M folio, we just
> check if we can charge atleast 1 folio. Are you saying that checking just 1 folio
> is not enough in this case and can still cause thrashing, i.e we should check more?

My understanding is that cgroups are hierarchical. Even if we don’t
hit the memory
 limit of the folio’s direct memcg, we could still reach the limit of
one of its parent
memcgs. Imagine a structure like:

/sys/fs/cgroup/a/b/c/d

If we’re compiling the kernel in d, there’s a chance that while d
isn’t at its limit, its
parents (c, b, or a) could be. Currently, the check only applies to d.

>
> If we want to maintain consitency for all folios another option is
> mem_cgroup_margin(memcg) < MEMCG_CHARGE_BATCH * folio_nr_pages(folio)
> but I think this is too extreme, we would be checking if 64M can be charged to
> cgroup just to swapin 1M.
>
> >
> > I believe there’s still room for improvement. For example, if a 64KB charge
> > attempt fails, there’s no need to waste time trying 32KB or 16KB. We can
> > directly fall back to 4KB, as 32KB and 16KB will also fail based on our
> > margin detection logic.
> >
>
> Yes that makes sense. Would something like below work to fix that:
>
> 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;
> diff --git a/mm/memory.c b/mm/memory.c
> index fecdd044bc0b..b6ce6605dc63 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4123,6 +4123,7 @@ static struct folio *alloc_swap_folio(struct vm_fault *vmf)
>         pte_t *pte;
>         gfp_t gfp;
>         int order;
> +       int ret;
>
>         /*
>          * If uffd is active for the vma we need per-page fault fidelity to
> @@ -4170,9 +4171,13 @@ static struct folio *alloc_swap_folio(struct vm_fault *vmf)
>                 addr = ALIGN_DOWN(vmf->address, PAGE_SIZE << order);
>                 folio = vma_alloc_folio(gfp, order, vma, addr, true);
>                 if (folio) {
> -                       if (!mem_cgroup_swapin_charge_folio(folio, vma->vm_mm,
> -                                                           gfp, entry))
> +                       ret = mem_cgroup_swapin_charge_folio(folio, vma->vm_mm, gfp, entry);
> +                       if (!ret) {
>                                 return folio;
> +                       } else if (ret == -ENOMEM) {
> +                               folio_put(folio);
> +                               goto fallback;
> +                       }
>                         folio_put(folio);
>                 }
>                 order = next_order(&orders, order);
>

Yes, does it make your kernel build even faster?

Thanks
Barry





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux