Re: [PATCH v6 01/15] mm: khugepaged: don't carry huge page to the next loop for !CONFIG_NUMA

[Yang Shi <shy828301@gmail.com>]

On Fri, Jun 3, 2022 at 5:40 PM Zach O'Keefe <zokeefe@google.com> wrote:
>
> From: Yang Shi <shy828301@gmail.com>
>
> The khugepaged has optimization to reduce huge page allocation calls for
> !CONFIG_NUMA by carrying the allocated but failed to collapse huge page to
> the next loop.  CONFIG_NUMA doesn't do so since the next loop may try to
> collapse huge page from a different node, so it doesn't make too much sense
> to carry it.
>
> But when NUMA=n, the huge page is allocated by khugepaged_prealloc_page()
> before scanning the address space, so it means huge page may be allocated
> even though there is no suitable range for collapsing.  Then the page would
> be just freed if khugepaged already made enough progress.  This could make
> NUMA=n run have 5 times as much thp_collapse_alloc as NUMA=y run.  This
> problem actually makes things worse due to the way more pointless THP
> allocations and makes the optimization pointless.
>
> This could be fixed by carrying the huge page across scans, but it will
> complicate the code further and the huge page may be carried
> indefinitely.  But if we take one step back,  the optimization itself seems
> not worth keeping nowadays since:
>   * Not too many users build NUMA=n kernel nowadays even though the kernel is
>     actually running on a non-NUMA machine. Some small devices may run NUMA=n
>     kernel, but I don't think they actually use THP.
>   * Since commit 44042b449872 ("mm/page_alloc: allow high-order pages to be
>     stored on the per-cpu lists"), THP could be cached by pcp.  This actually
>     somehow does the job done by the optimization.
>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> Signed-off-by: Zach O'Keefe <zokeefe@google.com>

Thanks for bringing the patch into the series. You could have my SOB
on this patch.

> ---
>  mm/khugepaged.c | 100 ++++++++----------------------------------------
>  1 file changed, 17 insertions(+), 83 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 476d79360101..cc3d6fb446d5 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -833,29 +833,30 @@ static int khugepaged_find_target_node(void)
>         last_khugepaged_target_node = target_node;
>         return target_node;
>  }
> +#else
> +static int khugepaged_find_target_node(void)
> +{
> +       return 0;
> +}
> +#endif
>
> -static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
> +/* Sleep for the first alloc fail, break the loop for the second fail */
> +static bool alloc_fail_should_sleep(struct page **hpage, bool *wait)
>  {
>         if (IS_ERR(*hpage)) {
>                 if (!*wait)
> -                       return false;
> +                       return true;
>
>                 *wait = false;
>                 *hpage = NULL;
>                 khugepaged_alloc_sleep();
> -       } else if (*hpage) {
> -               put_page(*hpage);
> -               *hpage = NULL;
>         }
> -
> -       return true;
> +       return false;
>  }
>
>  static struct page *
>  khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
>  {
> -       VM_BUG_ON_PAGE(*hpage, *hpage);
> -
>         *hpage = __alloc_pages_node(node, gfp, HPAGE_PMD_ORDER);
>         if (unlikely(!*hpage)) {
>                 count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
> @@ -867,74 +868,6 @@ khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
>         count_vm_event(THP_COLLAPSE_ALLOC);
>         return *hpage;
>  }
> -#else
> -static int khugepaged_find_target_node(void)
> -{
> -       return 0;
> -}
> -
> -static inline struct page *alloc_khugepaged_hugepage(void)
> -{
> -       struct page *page;
> -
> -       page = alloc_pages(alloc_hugepage_khugepaged_gfpmask(),
> -                          HPAGE_PMD_ORDER);
> -       if (page)
> -               prep_transhuge_page(page);
> -       return page;
> -}
> -
> -static struct page *khugepaged_alloc_hugepage(bool *wait)
> -{
> -       struct page *hpage;
> -
> -       do {
> -               hpage = alloc_khugepaged_hugepage();
> -               if (!hpage) {
> -                       count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
> -                       if (!*wait)
> -                               return NULL;
> -
> -                       *wait = false;
> -                       khugepaged_alloc_sleep();
> -               } else
> -                       count_vm_event(THP_COLLAPSE_ALLOC);
> -       } while (unlikely(!hpage) && likely(khugepaged_enabled()));
> -
> -       return hpage;
> -}
> -
> -static bool khugepaged_prealloc_page(struct page **hpage, bool *wait)
> -{
> -       /*
> -        * If the hpage allocated earlier was briefly exposed in page cache
> -        * before collapse_file() failed, it is possible that racing lookups
> -        * have not yet completed, and would then be unpleasantly surprised by
> -        * finding the hpage reused for the same mapping at a different offset.
> -        * Just release the previous allocation if there is any danger of that.
> -        */
> -       if (*hpage && page_count(*hpage) > 1) {
> -               put_page(*hpage);
> -               *hpage = NULL;
> -       }
> -
> -       if (!*hpage)
> -               *hpage = khugepaged_alloc_hugepage(wait);
> -
> -       if (unlikely(!*hpage))
> -               return false;
> -
> -       return true;
> -}
> -
> -static struct page *
> -khugepaged_alloc_page(struct page **hpage, gfp_t gfp, int node)
> -{
> -       VM_BUG_ON(!*hpage);
> -
> -       return  *hpage;
> -}
> -#endif
>
>  /*
>   * If mmap_lock temporarily dropped, revalidate vma
> @@ -1188,8 +1121,10 @@ static void collapse_huge_page(struct mm_struct *mm,
>  out_up_write:
>         mmap_write_unlock(mm);
>  out_nolock:
> -       if (!IS_ERR_OR_NULL(*hpage))
> +       if (!IS_ERR_OR_NULL(*hpage)) {
>                 mem_cgroup_uncharge(page_folio(*hpage));
> +               put_page(*hpage);
> +       }
>         trace_mm_collapse_huge_page(mm, isolated, result);
>         return;
>  }
> @@ -1992,8 +1927,10 @@ static void collapse_file(struct mm_struct *mm,
>         unlock_page(new_page);
>  out:
>         VM_BUG_ON(!list_empty(&pagelist));
> -       if (!IS_ERR_OR_NULL(*hpage))
> +       if (!IS_ERR_OR_NULL(*hpage)) {
>                 mem_cgroup_uncharge(page_folio(*hpage));
> +               put_page(*hpage);
> +       }
>         /* TODO: tracepoints */
>  }
>
> @@ -2243,7 +2180,7 @@ static void khugepaged_do_scan(void)
>         lru_add_drain_all();
>
>         while (progress < pages) {
> -               if (!khugepaged_prealloc_page(&hpage, &wait))
> +               if (alloc_fail_should_sleep(&hpage, &wait))
>                         break;
>
>                 cond_resched();
> @@ -2262,9 +2199,6 @@ static void khugepaged_do_scan(void)
>                         progress = pages;
>                 spin_unlock(&khugepaged_mm_lock);
>         }
> -
> -       if (!IS_ERR_OR_NULL(hpage))
> -               put_page(hpage);
>  }
>
>  static bool khugepaged_should_wakeup(void)
> --
> 2.36.1.255.ge46751e96f-goog
>