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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <28D1C313-8333-4F87-9B1D-47E77789A853@nvidia.com>
Date: Tue, 19 Mar 2024 21:16:37 -0400
From: Zi Yan <ziy@...dia.com>
To: SeongJae Park <sj@...nel.org>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
 "\"Matthew Wilcox (Oracle)\"" <willy@...radead.org>,
 Yang Shi <shy828301@...il.com>, Huang Ying <ying.huang@...el.com>,
 "\"Kirill A . Shutemov\"" <kirill.shutemov@...ux.intel.com>,
 Ryan Roberts <ryan.roberts@....com>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>,
 "\"Yin, Fengwei\"" <fengwei.yin@...el.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] mm/migrate: split source folio if it is on deferred
 split list

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@...t.com> wrote:
>>
>>> From: Zi Yan <ziy@...dia.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@...dia.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

Download attachment "signature.asc" of type "application/pgp-signature" (855 bytes)