| Message ID | 20230721094043.2506691-2-fengwei.yin@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fix large folio for madvise_cold_or_pageout() | expand |
On Fri, Jul 21, 2023 at 3:41 AM Yin Fengwei <fengwei.yin@intel.com> wrote: > > The commit > 07e8c82b5eff ("madvise: convert madvise_cold_or_pageout_pte_range() to > use folios") replaced the page_mapcount() with folio_mapcount() to > check whether the folio is shared by other mapping. > > But it's not correct for large folio. folio_mapcount() returns the > total mapcount of large folio which is not suitable to detect whether > the folio is shared. > > Use folio_estimated_sharers() which returns a estimated number of > shares. That means it's not 100% correct. But it should be OK for > madvise case here. > > Signed-off-by: Yin Fengwei <fengwei.yin@intel.com> Fixes: Cc: stable > @@ -383,7 +383,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, > folio = pfn_folio(pmd_pfn(orig_pmd)); > > /* Do not interfere with other mappings of this folio */ > - if (folio_mapcount(folio) != 1) > + if (folio_estimated_sharers(folio) != 1) Strictly speaking, this isn't a bug. But it may be ok to include it in the same patch. > goto huge_unlock; > > if (pageout_anon_only_filter && !folio_test_anon(folio)) > @@ -459,7 +459,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, > if (folio_test_large(folio)) { > int err; > > - if (folio_mapcount(folio) != 1) > + if (folio_estimated_sharers(folio) != 1) > break; > if (pageout_anon_only_filter && !folio_test_anon(folio)) > break; > @@ -682,7 +682,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, What about madvise_free_huge_pmd()? Should it be changed as well so that it's consistent with the first change? Either change both or neither. > if (folio_test_large(folio)) { > int err; > > - if (folio_mapcount(folio) != 1) > + if (folio_estimated_sharers(folio) != 1) This is another bug fix and should be in a separate patch. > break; > if (!folio_trylock(folio)) > break; Please send two separate fixes, and then: Reviewed-by: Yu Zhao <yuzhao@google.com>
On 7/22/2023 2:57 AM, Yu Zhao wrote: > On Fri, Jul 21, 2023 at 3:41 AM Yin Fengwei <fengwei.yin@intel.com> wrote: >> >> The commit >> 07e8c82b5eff ("madvise: convert madvise_cold_or_pageout_pte_range() to >> use folios") replaced the page_mapcount() with folio_mapcount() to >> check whether the folio is shared by other mapping. >> >> But it's not correct for large folio. folio_mapcount() returns the >> total mapcount of large folio which is not suitable to detect whether >> the folio is shared. >> >> Use folio_estimated_sharers() which returns a estimated number of >> shares. That means it's not 100% correct. But it should be OK for >> madvise case here. >> >> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com> > > Fixes: > Cc: stable OK > >> @@ -383,7 +383,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, >> folio = pfn_folio(pmd_pfn(orig_pmd)); >> >> /* Do not interfere with other mappings of this folio */ >> - if (folio_mapcount(folio) != 1) >> + if (folio_estimated_sharers(folio) != 1) > > Strictly speaking, this isn't a bug. But it may be ok to include it in > the same patch. OK. I will drop the change for pmd. > >> goto huge_unlock; >> >> if (pageout_anon_only_filter && !folio_test_anon(folio)) >> @@ -459,7 +459,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, >> if (folio_test_large(folio)) { >> int err; >> >> - if (folio_mapcount(folio) != 1) >> + if (folio_estimated_sharers(folio) != 1) >> break; >> if (pageout_anon_only_filter && !folio_test_anon(folio)) >> break; >> @@ -682,7 +682,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, > > What about madvise_free_huge_pmd()? Should it be changed as well so > that it's consistent with the first change? Either change both or neither. > >> if (folio_test_large(folio)) { >> int err; >> >> - if (folio_mapcount(folio) != 1) >> + if (folio_estimated_sharers(folio) != 1) > > This is another bug fix and should be in a separate patch. OK. Will split to two patches. > >> break; >> if (!folio_trylock(folio)) >> break; > > Please send two separate fixes, and then: > > Reviewed-by: Yu Zhao <yuzhao@google.com> Thanks a lot. I will drop the mapcount() change for pmd and sent to patches for madvise_cold_or_pageout_pte_range() and madvise_free_pte_range(). Regards Yin, Fengwei
On Sun, Jul 23, 2023 at 6:27 AM Yin, Fengwei <fengwei.yin@intel.com> wrote: > > > > On 7/22/2023 2:57 AM, Yu Zhao wrote: > > On Fri, Jul 21, 2023 at 3:41 AM Yin Fengwei <fengwei.yin@intel.com> wrote: > >> > >> The commit > >> 07e8c82b5eff ("madvise: convert madvise_cold_or_pageout_pte_range() to > >> use folios") replaced the page_mapcount() with folio_mapcount() to > >> check whether the folio is shared by other mapping. > >> > >> But it's not correct for large folio. folio_mapcount() returns the > >> total mapcount of large folio which is not suitable to detect whether > >> the folio is shared. > >> > >> Use folio_estimated_sharers() which returns a estimated number of > >> shares. That means it's not 100% correct. But it should be OK for > >> madvise case here. > >> > >> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com> > > > > Fixes: > > Cc: stable > OK > > > > >> @@ -383,7 +383,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, > >> folio = pfn_folio(pmd_pfn(orig_pmd)); > >> > >> /* Do not interfere with other mappings of this folio */ > >> - if (folio_mapcount(folio) != 1) > >> + if (folio_estimated_sharers(folio) != 1) > > > > Strictly speaking, this isn't a bug. But it may be ok to include it in > > the same patch. > OK. I will drop the change for pmd. > > > > >> goto huge_unlock; > >> > >> if (pageout_anon_only_filter && !folio_test_anon(folio)) > >> @@ -459,7 +459,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, > >> if (folio_test_large(folio)) { > >> int err; > >> > >> - if (folio_mapcount(folio) != 1) > >> + if (folio_estimated_sharers(folio) != 1) > >> break; > >> if (pageout_anon_only_filter && !folio_test_anon(folio)) > >> break; > >> @@ -682,7 +682,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, > > > > What about madvise_free_huge_pmd()? Should it be changed as well so > > that it's consistent with the first change? Either change both or neither. > > > >> if (folio_test_large(folio)) { > >> int err; > >> > >> - if (folio_mapcount(folio) != 1) > >> + if (folio_estimated_sharers(folio) != 1) > > > > This is another bug fix and should be in a separate patch. > OK. Will split to two patches. > > > > >> break; > >> if (!folio_trylock(folio)) > >> break; > > > > Please send two separate fixes, and then: > > > > Reviewed-by: Yu Zhao <yuzhao@google.com> > Thanks a lot. I will drop the mapcount() change for pmd and sent to patches > for madvise_cold_or_pageout_pte_range() and madvise_free_pte_range(). I don't mind including the PMD changes. Either way works for me :)
diff --git a/mm/madvise.c b/mm/madvise.c index 38382a5d1e39..f12933ebcc24 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -383,7 +383,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, folio = pfn_folio(pmd_pfn(orig_pmd)); /* Do not interfere with other mappings of this folio */ - if (folio_mapcount(folio) != 1) + if (folio_estimated_sharers(folio) != 1) goto huge_unlock; if (pageout_anon_only_filter && !folio_test_anon(folio)) @@ -459,7 +459,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, if (folio_test_large(folio)) { int err; - if (folio_mapcount(folio) != 1) + if (folio_estimated_sharers(folio) != 1) break; if (pageout_anon_only_filter && !folio_test_anon(folio)) break; @@ -682,7 +682,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, if (folio_test_large(folio)) { int err; - if (folio_mapcount(folio) != 1) + if (folio_estimated_sharers(folio) != 1) break; if (!folio_trylock(folio)) break;
The commit 07e8c82b5eff ("madvise: convert madvise_cold_or_pageout_pte_range() to use folios") replaced the page_mapcount() with folio_mapcount() to check whether the folio is shared by other mapping. But it's not correct for large folio. folio_mapcount() returns the total mapcount of large folio which is not suitable to detect whether the folio is shared. Use folio_estimated_sharers() which returns a estimated number of shares. That means it's not 100% correct. But it should be OK for madvise case here. Signed-off-by: Yin Fengwei <fengwei.yin@intel.com> --- mm/madvise.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)