lkml.org 
[lkml]   [2023]   [Aug]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 0/2] don't use mapcount() to check large folio sharing
On 02.08.23 12:27, Ryan Roberts wrote:
> On 28/07/2023 17:13, Yin Fengwei wrote:
>> In madvise_cold_or_pageout_pte_range() and madvise_free_pte_range(),
>> folio_mapcount() is used to check whether the folio is shared. But it's
>> not correct as folio_mapcount() returns total mapcount of large folio.
>>
>> Use folio_estimated_sharers() here as the estimated number is enough.
>>
>> Yin Fengwei (2):
>> madvise: don't use mapcount() against large folio for sharing check
>> madvise: don't use mapcount() against large folio for sharing check
>>
>> mm/huge_memory.c | 2 +-
>> mm/madvise.c | 6 +++---
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>
> As a set of fixes, I agree this is definitely an improvement, so:
>
> Reviewed-By: Ryan Roberts
>
>
> But I have a couple of comments around further improvements;
>
> Once we have the scheme that David is working on to be able to provide precise
> exclusive vs shared info, we will probably want to move to that. Although that
> scheme will need access to the mm_struct of a process known to be mapping the

There are probably ways to work around lack of mm_struct, but it would
not be completely for free. But passing the mm_struct should probably be
an easy refactoring.

> folio. We have that info, but its not passed to folio_estimated_sharers() so we
> can't just reimplement folio_estimated_sharers() - we will need to rework these
> call sites again.

We should probably just have a

folio_maybe_mapped_shared()

with proper documentation. Nobody should care about the exact number.


If my scheme for anon pages makes it in, that would be precise for anon
pages and we could document that. Once we can handle pagecache pages as
well to get a precise answer, we could change to folio_mapped_shared()
and adjust the documentation.


I just saw

https://lkml.kernel.org/r/20230802095346.87449-1-wangkefeng.wang@huawei.com

that converts a lot of code to folio_estimated_sharers().


That patchset, for example, also does

total_mapcount(page) > 1 -> folio_estimated_sharers(folio) > 1

I'm not 100% sure what to think about that at this point. We eventually
add false negatives (actually shared but we fail to detect it) all over
the place, instead of having false positives (actually exclusive, but we
fail to detect it).

And that patch set doesn't even spell that out.


Maybe it's as good as we will get, especially if my scheme doesn't make
it in. But we should definitely spell that out.

--
Cheers,

David / dhildenb

\
 
 \ /
  Last update: 2023-08-02 12:49    [W:0.344 / U:1.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site