Re: [PATCH v5] mm: shrink skip folio mapped by an exiting process

[Barry Song]

On Mon, Jul 8, 2024 at 9:04 PM Zhiguo Jiang <justinjiang@xxxxxxxx> wrote:
>
> The releasing process of the non-shared anonymous folio mapped solely by
> an exiting process may go through two flows: 1) the anonymous folio is
> firstly is swaped-out into swapspace and transformed into a swp_entry
> in shrink_folio_list; 2) then the swp_entry is released in the process
> exiting flow. This will increase the cpu load of releasing a non-shared
> anonymous folio mapped solely by an exiting process, because the folio
> go through swap-out and the releasing the swapspace and swp_entry.
>
> When system is low memory, it is more likely to occur, because more
> backend applidatuions will be killed.
>
> The modification is that shrink skips the non-shared anonymous folio
> solely mapped by an exting process and the folio is only released
> directly in the process exiting flow, which will save swap-out time
> and alleviate the load of the process exiting.
>
> Signed-off-by: Zhiguo Jiang <justinjiang@xxxxxxxx>
> ---
>
> Change log:
> v4->v5:
> 1.Modify to skip non-shared anonymous folio only.
> 2.Update comments for pra->referenced = -1.
> v3->v4:
> 1.Modify that the unshared folios mapped only in exiting task are skip.
> v2->v3:
> Nothing.
> v1->v2:
> 1.The VM_EXITING added in v1 patch is removed, because it will fail
> to compile in 32-bit system.
>
>  mm/rmap.c   | 13 +++++++++++++
>  mm/vmscan.c |  7 ++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 26806b49a86f..5b5281d71dbb
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -843,6 +843,19 @@ static bool folio_referenced_one(struct folio *folio,
>         int referenced = 0;
>         unsigned long start = address, ptes = 0;
>
> +       /*
> +        * Skip the non-shared anonymous folio mapped solely by
> +        * the single exiting process, and release it directly
> +        * in the process exiting.
> +        */
> +       if ((!atomic_read(&vma->vm_mm->mm_users) ||
> +               test_bit(MMF_OOM_SKIP, &vma->vm_mm->flags)) &&
> +               folio_test_anon(folio) && folio_test_swapbacked(folio) &&
> +               !folio_likely_mapped_shared(folio)) {
> +               pra->referenced = -1;
> +               return false;
> +       }
> +
>         while (page_vma_mapped_walk(&pvmw)) {
>                 address = pvmw.address;

As David suggested, what about the below?

@@ -883,6 +870,21 @@ static bool folio_referenced_one(struct folio *folio,
                        continue;
                }

+               /*
+                * Skip the non-shared anonymous folio mapped solely by
+                * the single exiting process, and release it directly
+                * in the process exiting.
+                */
+               if ((!atomic_read(&vma->vm_mm->mm_users) ||
+                                       test_bit(MMF_OOM_SKIP,
&vma->vm_mm->flags)) &&
+                               folio_test_anon(folio) &&
folio_test_swapbacked(folio) &&
+                               !folio_likely_mapped_shared(folio)) {
+                       pra->referenced = -1;
+                       page_vma_mapped_walk_done(&pvmw);
+                       return false;
+               }
+
                if (pvmw.pte) {
                        if (lru_gen_enabled() &&
                            pte_young(ptep_get(pvmw.pte))) {


By the way, I am not convinced that using test_bit(MMF_OOM_SKIP,
&vma->vm_mm->flags) is
correct (I think it is wrong).   For example, global_init can directly have it:
                if (is_global_init(p)) {
                        can_oom_reap = false;
                        set_bit(MMF_OOM_SKIP, &mm->flags);
                        pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
                                        task_pid_nr(victim), victim->comm,
                                        task_pid_nr(p), p->comm);
                        continue;
                }

And exit_mmap() automatically has MMF_OOM_SKIP.

What is the purpose of this check? Is there a better way to determine
if a process is an
OOM target? What about check_stable_address_space() ?


>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 0761f91b407f..bae7a8bf6b3d
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -863,7 +863,12 @@ static enum folio_references folio_check_references(struct folio *folio,
>         if (vm_flags & VM_LOCKED)
>                 return FOLIOREF_ACTIVATE;
>
> -       /* rmap lock contention: rotate */
> +       /*
> +        * There are two cases to consider.
> +        * 1) Rmap lock contention: rotate.
> +        * 2) Skip the non-shared anonymous folio mapped solely by
> +        *    the single exiting process.
> +        */
>         if (referenced_ptes == -1)
>                 return FOLIOREF_KEEP;
>
> --
> 2.39.0
>

Thanks
Barry