Re: [PATCH -mm -v8 1/3] mm, THP, swap: Delay splitting THP during swap out

["Huang\, Ying" <ying.huang@intel.com>]

Hi, Johannes,

Johannes Weiner <hannes@cmpxchg.org> writes:

> Hi Huang,
>
> I reviewed this patch based on the feedback I already provided, but
> eventually gave up and rewrote it. Please take review feedback more
> seriously in the future.

Thanks a lot for your help!  I do respect all your review and effort.
The -v8 patch doesn't take all your comments, just because I thought we
have not reach consensus for some points and I want to use -v8 patch to
discuss them.

One concern I have before is whether to split THP firstly when swap
space or memcg swap is used up.  Now I think your solution is
acceptable. And if we receive any regression report for that in the
future, it's not very hard to deal with.

> Attached below is the reworked patch. Most changes are to the layering
> (page functions, cluster functions, range functions) so that we don't
> make the lowest swap range code require a notion of huge pages, or
> make the memcg page functions take size information that can be
> gathered from the page itself. I turned the config symbol into a
> generic THP_SWAP that can later be extended when we add 2MB IO. The
> rest is function naming, #ifdef removal etc.

For some #ifdef in swapfile.c, it is to avoid unnecessary code size
increase for !CONFIG_TRANSPARENT_HUGEPAGE or platform with THP swap
optimization disabled.  Is it an issue?

> Please review whether this is an acceptable version for you.

Yes.  It is good for me.  I will give it more test on next Monday.

[...]

> diff --git a/mm/Kconfig b/mm/Kconfig
> index c89f472b658c..660fb765bf7d 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -447,6 +447,18 @@ choice
>  	  benefit.
>  endchoice
>  
> +config ARCH_WANTS_THP_SWAP
> +       def_bool n
> +
> +config THP_SWAP
> +	def_bool y
> +	depends on TRANSPARENT_HUGEPAGE && ARCH_WANTS_THP_SWAP
> +	help
> +	  Swap transparent huge pages in one piece, without splitting.
> +	  XXX: For now this only does clustered swap space allocation.

Is 'XXX' here intended.

> +
> +	  For selection by architectures with reasonable THP sizes.
> +
>  config	TRANSPARENT_HUGE_PAGECACHE
>  	def_bool y
>  	depends on TRANSPARENT_HUGEPAGE
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index d14dd961f626..4a5c1ca21894 100644

[...]

Best Regards,
Huang, Ying