On 7/14/2023 10:08 AM, Yu Zhao wrote: > On Thu, Jul 13, 2023 at 9:06 AM Yin Fengwei <fengwei.yin@xxxxxxxxx> wrote: >> >> Current madvise_cold_or_pageout_pte_range() has two problems for >> large folio support: >> - Using folio_mapcount() with large folio prevent large folio from >> picking up. >> - If large folio is in the range requested, shouldn't split it >> in madvise_cold_or_pageout_pte_range(). >> >> Fix them by: >> - Use folio_estimated_sharers() with large folio >> - If large folio is in the range requested, don't split it. Leave >> to page reclaim phase. >> >> For large folio cross boundaries of requested range, skip it if it's >> page cache. Try to split it if it's anonymous folio. If splitting >> fails, skip it. > > For now, we may not want to change the existing semantic (heuristic). > IOW, we may want to stick to the "only owner" condition: > > - if (folio_mapcount(folio) != 1) > + if (folio_entire_mapcount(folio) || > + (any_page_within_range_has_mapcount > 1)) > > +Minchan Kim The folio_estimated_sharers() was discussed here: https://lore.kernel.org/linux-mm/20230118232219.27038-6-vishal.moola@xxxxxxxxx/ https://lore.kernel.org/linux-mm/20230124012210.13963-2-vishal.moola@xxxxxxxxx/ Yes. It's accurate to check each page of large folio. But it may be over killed in some cases (And I think madvise is one of the cases not necessary to be accurate. So folio_estimated_sharers() is enough. Correct me if I am wrong). > > Also there is an existing bug here: the later commit 07e8c82b5eff8 > ("madvise: convert madvise_cold_or_pageout_pte_range() to use folios") > is incorrect for sure; the original commit 9c276cc65a58f ("mm: > introduce MADV_COLD") seems incorrect too. > > +Vishal Moola (Oracle) > > The "any_page_within_range_has_mapcount" test above seems to be the > only correct to meet condition claimed by the comments, before or > after the folio conversion, assuming here a THP page means the > compound page without PMD mappings (PMD-split). Otherwise the test is > always false (if it's also PMD mapped somewhere else). > > /* > * Creating a THP page is expensive so split it only if we > * are sure it's worth. Split it if we are only owner. > */ > >> The main reason to call folio_referenced() is to clear the yong of >> conresponding PTEs. So in page reclaim phase, there is good chance >> the folio can be reclaimed. >> >> Signed-off-by: Yin Fengwei <fengwei.yin@xxxxxxxxx> >> --- >> This patch is based on mlock large folio support rfc2 as it depends >> on the folio_in_range() added by that patchset >> >> Also folio_op_size() can be unitfied with get_folio_mlock_step(). >> >> Testing done: >> - kselftest: No new regression introduced. >> >> mm/madvise.c | 133 ++++++++++++++++++++++++++++++++------------------- >> 1 file changed, 84 insertions(+), 49 deletions(-) > > Also the refactor looks fine to me but it'd be better if it's a separate patch. OK. I will split the bug fix and refactor to two different patches. Thanks. Regards Yin, Fengwei
