Re: [PATCH v3] mm/migrate: split source folio if it is on deferred split list

[Zi Yan <ziy@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 4354 bytes --]

On 19 Mar 2024, at 21:09, Zi Yan wrote:

> On 19 Mar 2024, at 21:08, SeongJae Park wrote:
>
>> Hello,
>>
>> On Tue, 19 Mar 2024 11:47:53 -0400 Zi Yan <zi.yan@sent.com> wrote:
>>
>>> From: Zi Yan <ziy@nvidia.com>
>>>
>>> If the source folio is on deferred split list, it is likely some subpages
>>> are not used. Split it before migration to avoid migrating unused subpages.
>>>
>>> Commit 616b8371539a6 ("mm: thp: enable thp migration in generic path")
>>> did not check if a THP is on deferred split list before migration, thus,
>>> the destination THP is never put on deferred split list even if the source
>>> THP might be. The opportunity of reclaiming free pages in a partially
>>> mapped THP during deferred list scanning is lost, but no other harmful
>>> consequence is present[1].
>>>
>>> From v2:
>>> 1. Split the source folio instead of migrating it (per Matthew Wilcox)[2].
>>>
>>> From v1:
>>> 1. Used dst to get correct deferred split list after migration
>>>    (per Ryan Roberts).
>>>
>>> [1]: https://lore.kernel.org/linux-mm/03CE3A00-917C-48CC-8E1C-6A98713C817C@nvidia.com/
>>> [2]: https://lore.kernel.org/linux-mm/Ze_P6xagdTbcu1Kz@casper.infradead.org/
>>>
>>> Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path")
>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>> ---
>>>  mm/huge_memory.c | 22 ------------------
>>>  mm/internal.h    | 23 +++++++++++++++++++
>>>  mm/migrate.c     | 60 +++++++++++++++++++++++++++++++++++++++---------
>>>  3 files changed, 72 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 9859aa4f7553..c6d4d0cdf4b3 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -766,28 +766,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>>>  	return pmd;
>>>  }
>>>
>>> -#ifdef CONFIG_MEMCG
>>> -static inline
>>> -struct deferred_split *get_deferred_split_queue(struct folio *folio)
>>> -{
>>> -	struct mem_cgroup *memcg = folio_memcg(folio);
>>> -	struct pglist_data *pgdat = NODE_DATA(folio_nid(folio));
>>> -
>>> -	if (memcg)
>>> -		return &memcg->deferred_split_queue;
>>> -	else
>>> -		return &pgdat->deferred_split_queue;
>>> -}
>>> -#else
>>> -static inline
>>> -struct deferred_split *get_deferred_split_queue(struct folio *folio)
>>> -{
>>> -	struct pglist_data *pgdat = NODE_DATA(folio_nid(folio));
>>> -
>>> -	return &pgdat->deferred_split_queue;
>>> -}
>>> -#endif
>>> -
>>>  void folio_prep_large_rmappable(struct folio *folio)
>>>  {
>>>  	if (!folio || !folio_test_large(folio))
>>> diff --git a/mm/internal.h b/mm/internal.h
>>> index d1c69119b24f..8fa36e84463a 100644
>>> --- a/mm/internal.h
>>> +++ b/mm/internal.h
>>> @@ -1107,6 +1107,29 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,
>>>  				   unsigned long addr, pmd_t *pmd,
>>>  				   unsigned int flags);
>>>
>>> +#ifdef CONFIG_MEMCG
>>> +static inline
>>> +struct deferred_split *get_deferred_split_queue(struct folio *folio)
>>> +{
>>> +	struct mem_cgroup *memcg = folio_memcg(folio);
>>> +	struct pglist_data *pgdat = NODE_DATA(folio_nid(folio));
>>> +
>>> +	if (memcg)
>>> +		return &memcg->deferred_split_queue;
>>> +	else
>>> +		return &pgdat->deferred_split_queue;
>>> +}
>>> +#else
>>> +static inline
>>> +struct deferred_split *get_deferred_split_queue(struct folio *folio)
>>> +{
>>> +	struct pglist_data *pgdat = NODE_DATA(folio_nid(folio));
>>> +
>>> +	return &pgdat->deferred_split_queue;
>>> +}
>>> +#endif
>>
>> I found this breaks the build when CONFIG_TRANSPARENT_HUGEPAGE is not set, with
>> below error:
>>
>>     .../lib/../mm/internal.h: In function 'get_deferred_split_queue':
>>     .../lib/../mm/internal.h:1127:22: error: 'struct pglist_data' has no member named 'deferred_split_queue'
>>      1127 |         return &pgdat->deferred_split_queue;
>>           |                      ^~
>>
>> Since the code was in hugepage.c, maybe the above chunk need to be wrapped by
>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE?  I confirmed below change is fixing the
>> build on my setup.
>
> Thanks. Will fix it in the next version.

Actually, since get_deferred_split_queue() is used in mm/migrate.c, that
part needs to be guarded by CONFIG_TRANSPARENT_HUGEPAGE as well.

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]