Commit 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a
folio") replaced the page_mapcount() with folio_mapcount() to check
whether the folio is shared by other mapping.

It's not correct for large folios. 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. It should be OK for madvise case here.

User-visible effects is that the THP is skipped when user call madvise.
But the correct behavior is THP should be split and processed then.

NOTE: this change is a temporary fix to reduce the user-visible effects
before the long term fix from David is ready.

Fixes: 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a folio")
Cc: stable@vger.kernel.org
Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
Reviewed-by: Yu Zhao <yuzhao@google.com>
Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
---
 mm/madvise.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 49af35e2d99a..4dded5d27e7e 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -683,7 +683,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;
-- 
2.39.2

Hi Yin,
On Tue, Aug 08, 2023 at 10:09:17AM +0800, Yin Fengwei wrote:
> Commit 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a
> folio") replaced the page_mapcount() with folio_mapcount() to check
> whether the folio is shared by other mapping.
>
> It's not correct for large folios. 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. It should be OK for madvise case here.

I'm trying to understand why it should be ok for madvise this change, so
I hope it's okay to ask you few questions.

folio_mapcount() calculates the total maps for all the subpages of a
folio. However, the folio_estimated_sharers does it only for the first
subpage making it not true for large folios. Then, wouldn't this change
drop support for large folios?

Seems like folio_entire_mapcount() is not accurate either because of it
does not inclue PTE-mapped sub-pages which I think we need here. Hence,
the folio_mapcount(). Could this be something missing in the test side?

I tried to replicate the setup with CONFIG_TRANSPARENT_HUGEPAGE but
seems like I'm not able to do it:

./cow
# [INFO] detected THP size: 2048 KiB
# [INFO] detected hugetlb size: 2048 KiB
# [INFO] detected hugetlb size: 1048576 KiB
# [INFO] huge zeropage is enabled
TAP version 13
1..166
# [INFO] Anonymous memory tests in private mappings
# [RUN] Basic COW after fork() ... with base page
not ok 1 MADV_NOHUGEPAGE failed
# [RUN] Basic COW after fork() ... with swapped out base page
not ok 2 MADV_NOHUGEPAGE failed
# [RUN] Basic COW after fork() ... with THP
not ok 3 MADV_HUGEPAGE failed
# [RUN] Basic COW after fork() ... with swapped-out THP
not ok 4 MADV_HUGEPAGE failed
# [RUN] Basic COW after fork() ... with PTE-mapped THP
not ok 5 MADV_HUGEPAGE failed
# [RUN] Basic COW after fork() ... with swapped-out, PTE-mapped THP
not ok 6 MADV_HUGEPAGE failed
...


Daniel
>
> User-visible effects is that the THP is skipped when user call madvise.
> But the correct behavior is THP should be split and processed then.
>
> NOTE: this change is a temporary fix to reduce the user-visible effects
> before the long term fix from David is ready.
>
> Fixes: 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a folio")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> Reviewed-by: Yu Zhao <yuzhao@google.com>
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>  mm/madvise.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 49af35e2d99a..4dded5d27e7e 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -683,7 +683,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;
> --
> 2.39.2
>

On 8/15/23 21:25, Daniel Gomez wrote:
> Hi Yin,
> On Tue, Aug 08, 2023 at 10:09:17AM +0800, Yin Fengwei wrote:
>> Commit 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a
>> folio") replaced the page_mapcount() with folio_mapcount() to check
>> whether the folio is shared by other mapping.
>>
>> It's not correct for large folios. 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. It should be OK for madvise case here.
> 
> I'm trying to understand why it should be ok for madvise this change, so
> I hope it's okay to ask you few questions.
> 
> folio_mapcount() calculates the total maps for all the subpages of a
> folio. However, the folio_estimated_sharers does it only for the first
> subpage making it not true for large folios. Then, wouldn't this change
> drop support for large folios?
I saw David explained this very well in another mail.

> 
> Seems like folio_entire_mapcount() is not accurate either because of it
> does not inclue PTE-mapped sub-pages which I think we need here. Hence,
> the folio_mapcount(). Could this be something missing in the test side?
> 
> I tried to replicate the setup with CONFIG_TRANSPARENT_HUGEPAGE but
> seems like I'm not able to do it:
> 
> ./cow
> # [INFO] detected THP size: 2048 KiB
> # [INFO] detected hugetlb size: 2048 KiB
> # [INFO] detected hugetlb size: 1048576 KiB
> # [INFO] huge zeropage is enabled
> TAP version 13
> 1..166
> # [INFO] Anonymous memory tests in private mappings
> # [RUN] Basic COW after fork() ... with base page
> not ok 1 MADV_NOHUGEPAGE failed
> # [RUN] Basic COW after fork() ... with swapped out base page
> not ok 2 MADV_NOHUGEPAGE failed
> # [RUN] Basic COW after fork() ... with THP
> not ok 3 MADV_HUGEPAGE failed
> # [RUN] Basic COW after fork() ... with swapped-out THP
> not ok 4 MADV_HUGEPAGE failed
> # [RUN] Basic COW after fork() ... with PTE-mapped THP
> not ok 5 MADV_HUGEPAGE failed
> # [RUN] Basic COW after fork() ... with swapped-out, PTE-mapped THP
> not ok 6 MADV_HUGEPAGE failed
> ...
Can you post the MADV_PAGEOUT and PTE-mapped THP related testing result?
And I suppose swap need be enabled also for the testing.


Regards
Yin, Fengwei

> 
> 
> Daniel
>>
>> User-visible effects is that the THP is skipped when user call madvise.
>> But the correct behavior is THP should be split and processed then.
>>
>> NOTE: this change is a temporary fix to reduce the user-visible effects
>> before the long term fix from David is ready.
>>
>> Fixes: 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a folio")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
>> Reviewed-by: Yu Zhao <yuzhao@google.com>
>> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
>> ---
>>  mm/madvise.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 49af35e2d99a..4dded5d27e7e 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -683,7 +683,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;
>> --
>> 2.39.2
>>

On Wed, Aug 16, 2023 at 07:30:35AM +0800, Yin Fengwei wrote:
>
>
> On 8/15/23 21:25, Daniel Gomez wrote:
> > Hi Yin,
> > On Tue, Aug 08, 2023 at 10:09:17AM +0800, Yin Fengwei wrote:
> >> Commit 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a
> >> folio") replaced the page_mapcount() with folio_mapcount() to check
> >> whether the folio is shared by other mapping.
> >>
> >> It's not correct for large folios. 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. It should be OK for madvise case here.
> >
> > I'm trying to understand why it should be ok for madvise this change, so
> > I hope it's okay to ask you few questions.
> >
> > folio_mapcount() calculates the total maps for all the subpages of a
> > folio. However, the folio_estimated_sharers does it only for the first
> > subpage making it not true for large folios. Then, wouldn't this change
> > drop support for large folios?
> I saw David explained this very well in another mail.
>
> >
> > Seems like folio_entire_mapcount() is not accurate either because of it
> > does not inclue PTE-mapped sub-pages which I think we need here. Hence,
> > the folio_mapcount(). Could this be something missing in the test side?
> >
> > I tried to replicate the setup with CONFIG_TRANSPARENT_HUGEPAGE but
> > seems like I'm not able to do it:
> >
> > ./cow
> > # [INFO] detected THP size: 2048 KiB
> > # [INFO] detected hugetlb size: 2048 KiB
> > # [INFO] detected hugetlb size: 1048576 KiB
> > # [INFO] huge zeropage is enabled
> > TAP version 13
> > 1..166
> > # [INFO] Anonymous memory tests in private mappings
> > # [RUN] Basic COW after fork() ... with base page
> > not ok 1 MADV_NOHUGEPAGE failed
> > # [RUN] Basic COW after fork() ... with swapped out base page
> > not ok 2 MADV_NOHUGEPAGE failed
> > # [RUN] Basic COW after fork() ... with THP
> > not ok 3 MADV_HUGEPAGE failed
> > # [RUN] Basic COW after fork() ... with swapped-out THP
> > not ok 4 MADV_HUGEPAGE failed
> > # [RUN] Basic COW after fork() ... with PTE-mapped THP
> > not ok 5 MADV_HUGEPAGE failed
> > # [RUN] Basic COW after fork() ... with swapped-out, PTE-mapped THP
> > not ok 6 MADV_HUGEPAGE failed
> > ...
> Can you post the MADV_PAGEOUT and PTE-mapped THP related testing result?
> And I suppose swap need be enabled also for the testing.

You may find a dump of the logs in the link below with system information. Let me
know if you find something wrong in my setup or if you need something else.
Besides CONFIG_TRANSPARENT_HUGEPAGE, CONFIG_SWAP is also enabled in the kernel.

https://gitlab.com/-/snippets/2584135

Also, strace reports ENOSYS for MADV_*:
madvise(0x7f2912465000, 4096, MADV_NOHUGEPAGE) = -1 ENOSYS (Function not implemented)
madvise(0x7f2912000000, 2097152, MADV_HUGEPAGE) = -1 ENOSYS (Function not implemented)


>
>
> Regards
> Yin, Fengwei
>
> >
> >
> > Daniel
> >>
> >> User-visible effects is that the THP is skipped when user call madvise.
> >> But the correct behavior is THP should be split and processed then.
> >>
> >> NOTE: this change is a temporary fix to reduce the user-visible effects
> >> before the long term fix from David is ready.
> >>
> >> Fixes: 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a folio")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> >> Reviewed-by: Yu Zhao <yuzhao@google.com>
> >> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> >> ---
> >>  mm/madvise.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/mm/madvise.c b/mm/madvise.c
> >> index 49af35e2d99a..4dded5d27e7e 100644
> >> --- a/mm/madvise.c
> >> +++ b/mm/madvise.c
> >> @@ -683,7 +683,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;
> >> --
> >> 2.39.2
> >>

On 8/16/2023 7:44 PM, Daniel Gomez wrote:
> On Wed, Aug 16, 2023 at 07:30:35AM +0800, Yin Fengwei wrote:
>>
>>
>> On 8/15/23 21:25, Daniel Gomez wrote:
>>> Hi Yin,
>>> On Tue, Aug 08, 2023 at 10:09:17AM +0800, Yin Fengwei wrote:
>>>> Commit 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a
>>>> folio") replaced the page_mapcount() with folio_mapcount() to check
>>>> whether the folio is shared by other mapping.
>>>>
>>>> It's not correct for large folios. 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. It should be OK for madvise case here.
>>>
>>> I'm trying to understand why it should be ok for madvise this change, so
>>> I hope it's okay to ask you few questions.
>>>
>>> folio_mapcount() calculates the total maps for all the subpages of a
>>> folio. However, the folio_estimated_sharers does it only for the first
>>> subpage making it not true for large folios. Then, wouldn't this change
>>> drop support for large folios?
>> I saw David explained this very well in another mail.
>>
>>>
>>> Seems like folio_entire_mapcount() is not accurate either because of it
>>> does not inclue PTE-mapped sub-pages which I think we need here. Hence,
>>> the folio_mapcount(). Could this be something missing in the test side?
>>>
>>> I tried to replicate the setup with CONFIG_TRANSPARENT_HUGEPAGE but
>>> seems like I'm not able to do it:
>>>
>>> ./cow
>>> # [INFO] detected THP size: 2048 KiB
>>> # [INFO] detected hugetlb size: 2048 KiB
>>> # [INFO] detected hugetlb size: 1048576 KiB
>>> # [INFO] huge zeropage is enabled
>>> TAP version 13
>>> 1..166
>>> # [INFO] Anonymous memory tests in private mappings
>>> # [RUN] Basic COW after fork() ... with base page
>>> not ok 1 MADV_NOHUGEPAGE failed
>>> # [RUN] Basic COW after fork() ... with swapped out base page
>>> not ok 2 MADV_NOHUGEPAGE failed
>>> # [RUN] Basic COW after fork() ... with THP
>>> not ok 3 MADV_HUGEPAGE failed
>>> # [RUN] Basic COW after fork() ... with swapped-out THP
>>> not ok 4 MADV_HUGEPAGE failed
>>> # [RUN] Basic COW after fork() ... with PTE-mapped THP
>>> not ok 5 MADV_HUGEPAGE failed
>>> # [RUN] Basic COW after fork() ... with swapped-out, PTE-mapped THP
>>> not ok 6 MADV_HUGEPAGE failed
>>> ...
>> Can you post the MADV_PAGEOUT and PTE-mapped THP related testing result?
>> And I suppose swap need be enabled also for the testing.
> 
> You may find a dump of the logs in the link below with system information. Let me
> know if you find something wrong in my setup or if you need something else.
> Besides CONFIG_TRANSPARENT_HUGEPAGE, CONFIG_SWAP is also enabled in the kernel.
> 
> https://gitlab.com/-/snippets/2584135
> 
> Also, strace reports ENOSYS for MADV_*:
> madvise(0x7f2912465000, 4096, MADV_NOHUGEPAGE) = -1 ENOSYS (Function not implemented)
> madvise(0x7f2912000000, 2097152, MADV_HUGEPAGE) = -1 ENOSYS (Function not implemented)
O. The problem here is MADV_HUGEPAGE/MADV_NOHUGEPAGE doesn't work.
Do you have CONFIG_ADVISE_SYSCALLS enabled?


Regards
Yin, Fengwei

> 
> 
>>
>>
>> Regards
>> Yin, Fengwei
>>
>>>
>>>
>>> Daniel
>>>>
>>>> User-visible effects is that the THP is skipped when user call madvise.
>>>> But the correct behavior is THP should be split and processed then.
>>>>
>>>> NOTE: this change is a temporary fix to reduce the user-visible effects
>>>> before the long term fix from David is ready.
>>>>
>>>> Fixes: 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a folio")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
>>>> Reviewed-by: Yu Zhao <yuzhao@google.com>
>>>> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
>>>> ---
>>>>  mm/madvise.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/madvise.c b/mm/madvise.c
>>>> index 49af35e2d99a..4dded5d27e7e 100644
>>>> --- a/mm/madvise.c
>>>> +++ b/mm/madvise.c
>>>> @@ -683,7 +683,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;
>>>> --
>>>> 2.39.2
>>> >

On Wed, Aug 16, 2023 at 08:04:11PM +0800, Yin, Fengwei wrote:
>
>
> On 8/16/2023 7:44 PM, Daniel Gomez wrote:
> > On Wed, Aug 16, 2023 at 07:30:35AM +0800, Yin Fengwei wrote:
> >>
> >>
> >> On 8/15/23 21:25, Daniel Gomez wrote:
> >>> Hi Yin,
> >>> On Tue, Aug 08, 2023 at 10:09:17AM +0800, Yin Fengwei wrote:
> >>>> Commit 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a
> >>>> folio") replaced the page_mapcount() with folio_mapcount() to check
> >>>> whether the folio is shared by other mapping.
> >>>>
> >>>> It's not correct for large folios. 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. It should be OK for madvise case here.
> >>>
> >>> I'm trying to understand why it should be ok for madvise this change, so
> >>> I hope it's okay to ask you few questions.
> >>>
> >>> folio_mapcount() calculates the total maps for all the subpages of a
> >>> folio. However, the folio_estimated_sharers does it only for the first
> >>> subpage making it not true for large folios. Then, wouldn't this change
> >>> drop support for large folios?
> >> I saw David explained this very well in another mail.
> >>
> >>>
> >>> Seems like folio_entire_mapcount() is not accurate either because of it
> >>> does not inclue PTE-mapped sub-pages which I think we need here. Hence,
> >>> the folio_mapcount(). Could this be something missing in the test side?
> >>>
> >>> I tried to replicate the setup with CONFIG_TRANSPARENT_HUGEPAGE but
> >>> seems like I'm not able to do it:
> >>>
> >>> ./cow
> >>> # [INFO] detected THP size: 2048 KiB
> >>> # [INFO] detected hugetlb size: 2048 KiB
> >>> # [INFO] detected hugetlb size: 1048576 KiB
> >>> # [INFO] huge zeropage is enabled
> >>> TAP version 13
> >>> 1..166
> >>> # [INFO] Anonymous memory tests in private mappings
> >>> # [RUN] Basic COW after fork() ... with base page
> >>> not ok 1 MADV_NOHUGEPAGE failed
> >>> # [RUN] Basic COW after fork() ... with swapped out base page
> >>> not ok 2 MADV_NOHUGEPAGE failed
> >>> # [RUN] Basic COW after fork() ... with THP
> >>> not ok 3 MADV_HUGEPAGE failed
> >>> # [RUN] Basic COW after fork() ... with swapped-out THP
> >>> not ok 4 MADV_HUGEPAGE failed
> >>> # [RUN] Basic COW after fork() ... with PTE-mapped THP
> >>> not ok 5 MADV_HUGEPAGE failed
> >>> # [RUN] Basic COW after fork() ... with swapped-out, PTE-mapped THP
> >>> not ok 6 MADV_HUGEPAGE failed
> >>> ...
> >> Can you post the MADV_PAGEOUT and PTE-mapped THP related testing result?
> >> And I suppose swap need be enabled also for the testing.
> >
> > You may find a dump of the logs in the link below with system information. Let me
> > know if you find something wrong in my setup or if you need something else.
> > Besides CONFIG_TRANSPARENT_HUGEPAGE, CONFIG_SWAP is also enabled in the kernel.
> >
> > https://gitlab.com/-/snippets/2584135
> >
> > Also, strace reports ENOSYS for MADV_*:
> > madvise(0x7f2912465000, 4096, MADV_NOHUGEPAGE) = -1 ENOSYS (Function not implemented)
> > madvise(0x7f2912000000, 2097152, MADV_HUGEPAGE) = -1 ENOSYS (Function not implemented)
> O. The problem here is MADV_HUGEPAGE/MADV_NOHUGEPAGE doesn't work.
> Do you have CONFIG_ADVISE_SYSCALLS enabled?
It worked after I enabled the conf. Some tests failed and some were
skipped. But I managed to reproduce the issue now, thanks Yin!

Bail out! 4 out of 166 tests failed
# Totals: pass:146 fail:4 xfail:0 xpass:0 skip:16 error:0

Here the full log:
https://gitlab.com/-/snippets/2584190/raw/main/cow.txt
>
> Regards
> Yin, Fengwei
>
> >
> >
> >>
> >>
> >> Regards
> >> Yin, Fengwei
> >>
> >>>
> >>>
> >>> Daniel
> >>>>
> >>>> User-visible effects is that the THP is skipped when user call madvise.
> >>>> But the correct behavior is THP should be split and processed then.
> >>>>
> >>>> NOTE: this change is a temporary fix to reduce the user-visible effects
> >>>> before the long term fix from David is ready.
> >>>>
> >>>> Fixes: 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a folio")
> >>>> Cc: stable@vger.kernel.org
> >>>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> >>>> Reviewed-by: Yu Zhao <yuzhao@google.com>
> >>>> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> >>>> ---
> >>>>  mm/madvise.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/mm/madvise.c b/mm/madvise.c
> >>>> index 49af35e2d99a..4dded5d27e7e 100644
> >>>> --- a/mm/madvise.c
> >>>> +++ b/mm/madvise.c
> >>>> @@ -683,7 +683,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;
> >>>> --
> >>>> 2.39.2
> >>> >

On 16.08.23 16:13, Daniel Gomez wrote:
> On Wed, Aug 16, 2023 at 08:04:11PM +0800, Yin, Fengwei wrote:
>>
>>
>> On 8/16/2023 7:44 PM, Daniel Gomez wrote:
>>> On Wed, Aug 16, 2023 at 07:30:35AM +0800, Yin Fengwei wrote:
>>>>
>>>>
>>>> On 8/15/23 21:25, Daniel Gomez wrote:
>>>>> Hi Yin,
>>>>> On Tue, Aug 08, 2023 at 10:09:17AM +0800, Yin Fengwei wrote:
>>>>>> Commit 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a
>>>>>> folio") replaced the page_mapcount() with folio_mapcount() to check
>>>>>> whether the folio is shared by other mapping.
>>>>>>
>>>>>> It's not correct for large folios. 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. It should be OK for madvise case here.
>>>>>
>>>>> I'm trying to understand why it should be ok for madvise this change, so
>>>>> I hope it's okay to ask you few questions.
>>>>>
>>>>> folio_mapcount() calculates the total maps for all the subpages of a
>>>>> folio. However, the folio_estimated_sharers does it only for the first
>>>>> subpage making it not true for large folios. Then, wouldn't this change
>>>>> drop support for large folios?
>>>> I saw David explained this very well in another mail.
>>>>
>>>>>
>>>>> Seems like folio_entire_mapcount() is not accurate either because of it
>>>>> does not inclue PTE-mapped sub-pages which I think we need here. Hence,
>>>>> the folio_mapcount(). Could this be something missing in the test side?
>>>>>
>>>>> I tried to replicate the setup with CONFIG_TRANSPARENT_HUGEPAGE but
>>>>> seems like I'm not able to do it:
>>>>>
>>>>> ./cow
>>>>> # [INFO] detected THP size: 2048 KiB
>>>>> # [INFO] detected hugetlb size: 2048 KiB
>>>>> # [INFO] detected hugetlb size: 1048576 KiB
>>>>> # [INFO] huge zeropage is enabled
>>>>> TAP version 13
>>>>> 1..166
>>>>> # [INFO] Anonymous memory tests in private mappings
>>>>> # [RUN] Basic COW after fork() ... with base page
>>>>> not ok 1 MADV_NOHUGEPAGE failed
>>>>> # [RUN] Basic COW after fork() ... with swapped out base page
>>>>> not ok 2 MADV_NOHUGEPAGE failed
>>>>> # [RUN] Basic COW after fork() ... with THP
>>>>> not ok 3 MADV_HUGEPAGE failed
>>>>> # [RUN] Basic COW after fork() ... with swapped-out THP
>>>>> not ok 4 MADV_HUGEPAGE failed
>>>>> # [RUN] Basic COW after fork() ... with PTE-mapped THP
>>>>> not ok 5 MADV_HUGEPAGE failed
>>>>> # [RUN] Basic COW after fork() ... with swapped-out, PTE-mapped THP
>>>>> not ok 6 MADV_HUGEPAGE failed
>>>>> ...
>>>> Can you post the MADV_PAGEOUT and PTE-mapped THP related testing result?
>>>> And I suppose swap need be enabled also for the testing.
>>>
>>> You may find a dump of the logs in the link below with system information. Let me
>>> know if you find something wrong in my setup or if you need something else.
>>> Besides CONFIG_TRANSPARENT_HUGEPAGE, CONFIG_SWAP is also enabled in the kernel.
>>>
>>> https://gitlab.com/-/snippets/2584135
>>>
>>> Also, strace reports ENOSYS for MADV_*:
>>> madvise(0x7f2912465000, 4096, MADV_NOHUGEPAGE) = -1 ENOSYS (Function not implemented)
>>> madvise(0x7f2912000000, 2097152, MADV_HUGEPAGE) = -1 ENOSYS (Function not implemented)
>> O. The problem here is MADV_HUGEPAGE/MADV_NOHUGEPAGE doesn't work.
>> Do you have CONFIG_ADVISE_SYSCALLS enabled?
> It worked after I enabled the conf. Some tests failed and some were
> skipped. But I managed to reproduce the issue now, thanks Yin!
> 
> Bail out! 4 out of 166 tests failed
> # Totals: pass:146 fail:4 xfail:0 xpass:0 skip:16 error:0
> 

These hugetlb that are failing are known failures.

-- 
Cheers,

David / dhildenb

On 15.08.23 15:25, Daniel Gomez wrote:
> Hi Yin,
> On Tue, Aug 08, 2023 at 10:09:17AM +0800, Yin Fengwei wrote:
>> Commit 98b211d6415f ("madvise: convert madvise_free_pte_range() to use a
>> folio") replaced the page_mapcount() with folio_mapcount() to check
>> whether the folio is shared by other mapping.
>>
>> It's not correct for large folios. 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. It should be OK for madvise case here.
> 
> I'm trying to understand why it should be ok for madvise this change, so
> I hope it's okay to ask you few questions.
> 
> folio_mapcount() calculates the total maps for all the subpages of a
> folio. However, the folio_estimated_sharers does it only for the first
> subpage making it not true for large folios. Then, wouldn't this change
> drop support for large folios?

It's all a mess right now.

1) page_mapcount(page): how often it this page mapped

For a THP: entire mapcount of the THP (PMD-mapping) + mapcount of *this 
very subpage* (PTE-mapping) only


2) folio_mapcount(): how often is this folio mapped

For a THP: entire mapcount of the THP (PMD-mapping) + mapcount of *all* 
subpages (PTE-mapping) of the folio


3) folio_estimated_sharers(): how often is the first page mapped

For a THP: entire mapcount of the THP (PMD-mapping) + mapcount of *the 
first subpage* (PTE-mapping) only


For the time being, folio_estimated_sharers() is better then 
folio_mapcount(), because for a PTE-mapped THP folio_mapcount() > 1.

I'm looking into a replacement for folio_estimated_sharers() that is 
more precise ("folio_mapped_shared()"), but it's all a bit tricky. :)

-- 
Cheers,

David / dhildenb