On Fri, Jun 3, 2022 at 5:40 PM Zach O'Keefe <zokeefe@xxxxxxxxxx> wrote:
>
> From: Yang Shi <shy828301@xxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
>
> Signed-off-by: Zach O'Keefe <zokeefe@xxxxxxxxxx>
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
>
