Re: [PATCH 2/3] mm: changes to split_huge_page() to free zero filled tail pages

From: "Huang, Ying" <ying.huang@intel.com>
To: <alexlzhu@fb.com>
Cc: <linux-mm@kvack.org>, <willy@infradead.org>,
	<akpm@linux-foundation.org>, <riel@surriel.com>,
	<hannes@cmpxchg.org>, <linux-kernel@vger.kernel.org>,
	<kernel-team@fb.com>
Subject: Re: [PATCH 2/3] mm: changes to split_huge_page() to free zero filled tail pages
Date: Wed, 28 Sep 2022 16:48:40 +0800	[thread overview]
Message-ID: <87v8p728bb.fsf@yhuang6-desk2.ccr.corp.intel.com> (raw)
In-Reply-To: <94de34378bb748196e7709205a75331569d1d28e.1664347167.git.alexlzhu@fb.com> (alexlzhu@fb.com's message of "Tue, 27 Sep 2022 23:44:12 -0700")

<alexlzhu@fb.com> writes:

> From: Alexander Zhu <alexlzhu@fb.com>
>
> Currently, when /sys/kernel/mm/transparent_hugepage/enabled=always is set
> there are a large number of transparent hugepages that are almost entirely
> zero filled.  This is mentioned in a number of previous patchsets
> including:
> https://lore.kernel.org/all/20210731063938.1391602-1-yuzhao@google.com/
> https://lore.kernel.org/all/
> 1635422215-99394-1-git-send-email-ningzhang@linux.alibaba.com/
>
> Currently, split_huge_page() does not have a way to identify zero filled
> pages within the THP. Thus these zero pages get remapped and continue to
> create memory waste. In this patch, we identify and free tail pages that
> are zero filled in split_huge_page(). In this way, we avoid mapping these
> pages back into page table entries and can free up unused memory within
> THPs. This is based off the previously mentioned patchset by Yu Zhao.
> However, we chose to free anonymous zero tail pages whenever they are
> encountered instead of only on reclaim or migration.
>
> We also add self tests to verify the RssAnon value to make sure zero
> pages are not remapped except in the case of userfaultfd. In the case
> of userfaultfd we remap to the shared zero page, similar to what is
> done by KSM.
>
> Signed-off-by: Alexander Zhu <alexlzhu@fb.com>
> ---
>  include/linux/rmap.h                          |   2 +-
>  include/linux/vm_event_item.h                 |   3 +
>  mm/huge_memory.c                              |  44 ++++++-
>  mm/migrate.c                                  |  72 +++++++++--
>  mm/migrate_device.c                           |   4 +-
>  mm/vmstat.c                                   |   3 +
>  .../selftests/vm/split_huge_page_test.c       | 113 +++++++++++++++++-
>  tools/testing/selftests/vm/vm_util.c          |  23 ++++
>  tools/testing/selftests/vm/vm_util.h          |   1 +
>  9 files changed, 250 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index b89b4b86951f..f7d5d5639dea 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -372,7 +372,7 @@ int folio_mkclean(struct folio *);
>  int pfn_mkclean_range(unsigned long pfn, unsigned long nr_pages, pgoff_t pgoff,
>  		      struct vm_area_struct *vma);
>  
> -void remove_migration_ptes(struct folio *src, struct folio *dst, bool locked);
> +void remove_migration_ptes(struct folio *src, struct folio *dst, bool locked, bool unmap_clean);

There are 2 bool parameters now.  How about use "flags" style
parameters?  IMHO, well defined constants are more readable than a set
of true/false.

Best Regards,
Huang, Ying

[snip]