Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified

[David Hildenbrand <david@redhat.com>]

On 11.01.22 08:40, Alistair Popple wrote:
> On Monday, 10 January 2022 7:37:15 PM AEDT David Hildenbrand wrote:
>> On 15.11.21 14:49, Peter Xu wrote:
>>> This check existed since the 1st git commit of Linux repository, but at that
>>> time there's no page migration yet so I think it's okay.
>>>
>>> With page migration enabled, it should logically be possible that we zap some
>>> shmem pages during migration.  When that happens, IIUC the old code could have
>>> the RSS counter accounted wrong on MM_SHMEMPAGES because we will zap the ptes
>>> without decreasing the counters for the migrating entries.  I have no unit test
>>> to prove it as I don't know an easy way to trigger this condition, though.
>>>
>>> Besides, the optimization itself is already confusing IMHO to me in a few points:
>>>
>>>   - The wording "skip swap entries" is confusing, because we're not skipping all
>>>     swap entries - we handle device private/exclusive pages before that.
>>
>> I think one part of the confusion is "swap vs non-swap" entries.
>> For !pte_none() && !pte_present() we can have
>>
>> * swap entry
>> * non-swap entry
>> ** device exclusive entry
>> ** device private entry
>> ** HWpoison entry
>> ** migration entry
>>
>> So the comment claims to skip "swap entries" but also skips HWpoison and
>> migration entries, and I think that's the confusing part.
>> Both only apply to PageAnon().
> 
> I must be missing something but why do these only apply to PageAnon()?

My memory might be wrong. I remember that for PageAnon() we need
migration/hwpoison entries because there is no way we could refault the
page from a mapping once we zap the entry. For everything else, we could
zap and refault. But looks like we indeed also use migration/hwpoison
entries for pages with a mapping, although it might not be strictly
required.

> 
>> IIUC, the only way we could get details != NULL is via unmap_mapping_page()+unmap_mapping_pages().
>>
>> I do wonder if any of the callers really cares about PageAnon() pages where this would be relevant.
>>
>> Am I wrong or is unmap_mapping_pages() never called with "even_cows == true" and we can remove
>> that paremeter:
> 
> Except that unmap_mapping_range() takes `even_cows` as a parameter and passes
> that to unmap_mapping_pages(), and from what I can tell there are callers of
> unmap_mapping_range() that set `even_cows = true`.

You're right.

-- 
Thanks,

David / dhildenb