Re: All MADV_FREE mTHPs are fully subjected to deferred_split_folio()

[David Hildenbrand <david@redhat.com>]

On 30.12.24 21:22, Barry Song wrote:
> On Tue, Dec 31, 2024 at 8:32 AM David Hildenbrand <david@redhat.com> wrote:
>>
>>>> goto discard;
>>>>
>>>
>>> I agree that this is necessary, but I'm not sure it addresses my
>>> concerns. MADV_FREE'ed mTHPs are still being added to `deferred_split`,
>>> and this does not resolve the issue of them being partially unmapped
>>> though it is definitely better than the existing code, at least folios are
>>> not moved back to swap-backed.
>>   > > On the other hand, users might rely on the `deferred_split` counter to
>>> assess how aggressively userspace is performing address/size unaligned
>>> operations
>>> like MADV_DONTNEED or unmapped behavior. However, our debugging shows
>>> that the majority of `deferred_split` counter increments result from
>>> aligned MADV_FREE operations. This diminishes the counter's usefulness
>>> in reflecting unaligned userspace behavior.
>>
>> Optimizing that is certainly something to look into, but the bigger
>> issue you describe arises from bad handling of speculative references.
>>
>> Just imagine you indeed have a partially-mapped anon folio and the
>> remaining pages are MADV_FREE'ed. The problem with the speculative
>> reference would still apply.
>>
>>>
>>> If possible, I am still looking for some approach to entirely avoid
>>> adding the folio to deferred_split and partially being unmapped.
>>>
>>> Could the concept be something like this?
>>
>> Very likely it's wrong, because you really have to assure that that
>> folio range is mapped here.
>>
>> Proper folio PTE batching should be applied here -- folio_pte_batch() etc.
>>
> 
> I agree that using `folio_pte_batch()` to check if all PTEs are mapped and
> determining `any_dirty` for setting swap-backed is the right direction. I'm
> just curious if `(!list_empty(&folio->_deferred_list))` or
> `folio_test_partially_mapped(folio)` could replace it if we're aiming for
> a smaller change :-)

Likely we should just do it cleanly using PTE batching without any such 
optimizations. All unmapping (not just MADV_FREE) will benefit from PTE 
batching :)

> 
>> That can please the counters in many, but not all cases. Again, maybe
>> the deferred-split handling should be handled differently, and not
>> synchronously from rmap code.
>>
>> I see 3 different work items
>>
>> 1) Fix mis-handling of speculative references
>>
> 
> Agreed, the patch you're sending is absolutely necessary. I'd prefer
> it lands sooner in some way. Would you like to post it?

I'm out until the 7th. I'll be happy if you could clean it up, test it 
and send it out (Suggested-by: is sufficient).

> 
>> 2) Perform proper PTE batching during unmap/migration. Will improve
>>      performance in any case.
> 
> Agreed. I remember discussing this with Ryan in an email thread about
> a year ago, even for normal (non-MADV_FREE'ed) folios, but it seems
> everyone has been busy with other priorities.

Unfortunately, yes.

> 
> This seems like a good time to start exploring the idea. We could begin
> with MADV_FREE'ed folios and later extend it to normal folios—for
> instance, by implementing batched setting of swap entries.
> 
>>
>> 3) Try moving deferred-split handling out of rmap code into reclaim/
>>      access-bit handling.
> 
> I'm not quite sure we still need this after having 1 and 2. With those,
> we've been able to operate on the mTHP as a whole. Do we still
> need to move deferred_split out of rmap?

I have various things in mind, like having mremap'ed parts of the folio 
(cannot batch), user space zapping the folio in smaller chunks (cannot 
batch), and ... removing folio->_nr_pages_mapped + page->_mapcount, 
where we might not always detect "partially mapped" from rmap code and 
want to detect that separately either way.

Ideally, we'd just move deferred-split handling out of rmap code, and 
trigger that detection+handling from reclaim code. After all, the only 
purpose of deferred-split is .. memory reclaim.

Anyhow, 1) and 2) are beneficial independent of 3).

-- 
Cheers,

David / dhildenb