Date:   Fri, 14 Jul 2023 17:35:46 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Zi Yan <ziy@...dia.com>
Cc:     Yu Zhao <yuzhao@...gle.com>,
        "Yin, Fengwei" <fengwei.yin@...el.com>,
        Minchan Kim <minchan@...nel.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
        willy@...radead.org, ryan.roberts@....com, shy828301@...il.com,
        "Vishal Moola (Oracle)" <vishal.moola@...il.com>
Subject: Re: [RFC PATCH] madvise: make madvise_cold_or_pageout_pte_range()
 support large folio

On 14.07.23 16:41, Zi Yan wrote:
> On 14 Jul 2023, at 3:31, David Hildenbrand wrote:
> 
>> On 14.07.23 05:23, Yu Zhao wrote:
>>> On Thu, Jul 13, 2023 at 9:10 PM Yin, Fengwei <fengwei.yin@...el.com> wrote:
>>>>
>>>>
>>>>
>>>> On 7/14/2023 10:08 AM, Yu Zhao wrote:
>>>>> On Thu, Jul 13, 2023 at 9:06 AM Yin Fengwei <fengwei.yin@...el.com> 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@gmail.com/
>>>> https://lore.kernel.org/linux-mm/20230124012210.13963-2-vishal.moola@gmail.com/
>>>>
>>>> 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).
>>>
>>> I see. Then it's possible this is also what the original commit wants
>>> to do -- Minchan, could you clarify?
>>>
>>> Regardless, I think we can have the following fix, potentially cc'ing stable:
>>>
>>> -  if (folio_mapcount(folio) != 1)
>>> +  if (folio_estimated_sharers(folio) != 1)
>>>
>>> Sounds good?
>>
>> Adding to the discussion, currently the COW selftest always skips a PTE-mapped THP.
>>
>>
>> For example:
>>
>> # [INFO] Anonymous memory tests in private mappings
>> # [RUN] Basic COW after fork() ... with base page
>> ok 1 No leak from parent into child
>> # [RUN] Basic COW after fork() ... with swapped out base page
>> ok 2 No leak from parent into child
>> # [RUN] Basic COW after fork() ... with THP
>> ok 3 No leak from parent into child
>> # [RUN] Basic COW after fork() ... with swapped-out THP
>> ok 4 No leak from parent into child
>> # [RUN] Basic COW after fork() ... with PTE-mapped THP
>> ok 5 No leak from parent into child
>> # [RUN] Basic COW after fork() ... with swapped-out, PTE-mapped THP
>> ok 6 # SKIP MADV_PAGEOUT did not work, is swap enabled?
>> ...
>>
>>
>> The commit that introduced that change is:
>>
>> commit 07e8c82b5eff8ef34b74210eacb8d9c4a2886b82
>> Author: Vishal Moola (Oracle) <vishal.moola@...il.com>
>> Date:   Wed Dec 21 10:08:46 2022 -0800
>>
>>      madvise: convert madvise_cold_or_pageout_pte_range() to use folios
>>
>>      This change removes a number of calls to compound_head(), and saves
>>      1729 bytes of kernel text.
>>
>>
>>
>> folio_mapcount(folio) is wrong, because that never works on a PTE-mapped THP (well, unless only a single subpage is still mapped ...).
>>
>> page_mapcount(folio) was wrong, because it ignored all other subpages, but at least it worked in some cases.
>>
>> folio_estimated_sharers(folio) is similarly wrong like page_mapcount(), as it's essentially a page_mapcount() of the first subpage.
>>
>> (ignoring that a lockless mapcount-based check is always kind-of unreliable, but that's msotly acceptable for these kind of things)
>>
>>
>> So, unfortunately, page_mapcount() / folio_estimated_sharers() is best we can do for now, but they miss to detect some cases of sharing of the folio -- false negatives to detect sharing.
>>
>>
>> Ideally we want something like folio_maybe_mapped_shared(), and get rid of folio_estimated_sharers(), we better to guess the exact number, simply works towards an answer that tells us "yep, this may be mapped by multiple sharers" vs. "no, this is definitely not mapped by multiple sharers".
>>
>> The "mapped" part of it indicates that this does not catch all cases of sharing. But it should handle most of the cases we care about.
>>
>>
>> There, we can then implement something better than what folio_estimated_sharers() currently does:
>>
>> static inline bool folio_maybe_mapped_shared(folio)
>> {
>> 	if (likely(!folio_test_large(folio)))
>> 		return atomic_read(&folio->_mapcount) > 0;
>>
>> 	/* Mapped multiple times via PMD? */
>> 	if (folio_test_pmd_mappable(folio)
>> 		return folio_entire_mapcount() > 1;
>>
>> 	/*
>> 	 * First subpage is mapped multiple times (especially also via
>> 	 * PMDs)?
>>           */
>> 	if (page_mapcount(folio_page(folio, 0) > 1)
>> 		return true;
>>
>> 	/* TODO: also test last subpage? */
>> 	
>> 	/* Definitely shared if we're mapping a page multiple times. */
>> 	return folio_total_mapcount(folio) > folio_nr_pages(folio);
>> }
>>
>> There are some more things we could optimize for.
> 
> Before jumping into the mapcount, I would like to get some clarification
> on "sharer". Does it mean a page is mapped/shared by more than one page
> table entry or is mapped/shared by more than one process? Your function

:) I think it depends. For a order-0 page it is "more than one page 
table entry", which is is what the order-0 mapcount expresses.


So let's focus on order > 0 (and keep KSM out of the picture). There, it 
is different.

We're talking about "logical mapping" of the page. So a single logical 
mapping == one sharer.


1) Anon pages

For the time being, it really is "mapped by one process" -> exclusively 
mapped, "mapped by more than one process" -> mapped shared.

It's not "mapped into multiple page tables" or "mapped into multiple VMAs".

[That doesn't necessarily have to be that way for ever -- imagine we'd 
ever support a mremap(KEEP) with COW semantics -- but think there would 
be ways to handle that].


2) Pagecache pages

It really depends what we want. Either

(a) "mapped via a single logical mmap() operation"
      mapped.

(b) "mapped from a single open() operation"

(c) mapped by a single process.

Of course, mremap() is weird.


Currently, with the order-0 mapcount (and we cannot really change these 
semantics easily because we don't have any space to store additional 
information), it would be (a).

For example, mremap(KEEP) or a second mmap() creates a new logical mapping.

But the pagecache is kind-of weird: anybody could immediately map a page 
after we detected it as "exclusive" -- and eventually even concurrently. 
So this detection of "shared" is inherently problematic.


My primary focus is anon pages for now (as so often :) ). Anon is hard 
but eventually a bit easier to handle -- famous last words.

> indicates it is the former, but for madvise_cold_or_pageout_pte_range(),
> I am not sure that is what we want. What if user wants to page out a
> page that is mapped by the same process twice? With current method
> or any existing proposals, it will fail undesirably. It will only work
> as expect with your creator proposal[1].

For anon pages it's fairly easy IMHO. For pagecache pages, I guess we 
should keep it the way it was for order-0 pages: individual logical 
mmap() operations -- (a). It's sub-optimal, but the way it used to be 
for order-0.


> 
> Other places like auto NUMA migration also use mapcount to check if
> a page is mapped by multiple process with the assumption that a page will
> only be mapped by one process once.

Right, most of these cases might be able to use 
folio_maybe_mapped_shared() to detect "currently the whole folio is 
mapped exclusively by a single logical mapping". That makes a lot of 
sense in most cases that want to mess with a folio (e.g., migration, 
pageout, NUMA).

-- 
Cheers,

David / dhildenb

Powered by blists - more mailing lists