Re: [PATCH RFC 0/4] mm: Enable PM_SWAP for shmem with PTE_MARKER

[Tiberiu Georgescu]

> On 18 Aug 2021, at 09:24, David Hildenbrand <david@xxxxxxxxxx> wrote:
> 
> On 17.08.21 22:24, Peter Xu wrote:
>> On Tue, Aug 17, 2021 at 08:46:45PM +0200, David Hildenbrand wrote:
>>>> Please have a look at current pagemap impl in pte_to_pagemap_entry().  It's not
>>>> accurate from the 1st day, imho.  E.g., when a page is being migrated from numa
>>>> node 1 to node 2, we'll mark it PM_SWAP but I think it's not the case.  We can
>>>> make it more accurate, but I think it's fine, because it's a hint.
>>> 
>>> That inconsistency doesn't really matter as you can determine if something
>>> is present and worth dumping if it's either swapped or present. As long as
>>> it's one of both but not simply nothing.
>>> 
>>> I will shamelessly reference
>>> tools/testing/selftests/vm/madv_populate.c:pagemap_is_populated() that
>>> checks exactly for that (the test case uses only private anonymous memory).
>> Then I think the MADV_POPULATE_READ|WRITE test cases shouldn't depend on
>> PM_SWAP for that when it goes beyond anonymous private memories - when shmem
>> swapped out the pte can be none, then the test case can fail even if it
>> shouldn't, imho.
> 
> Exactly, because the pagemap is fairly completely broken for shmem.
> 
>> The mincore() syscall seems to be ideally the thing you may want to make it
>> accurate, but again it's not a problem for current anonymous private memories.
> 
> I haven't checked the details, but I believe the mincore() syscall won't report swapped out pages. At least according to its documentation:
> 
> "mincore()  returns a vector that indicates whether pages of the calling process's virtual memory are resident in core (RAM), and so will not cause a disk access  (page  fault)  if  referenced."
> 
> (to protect it from swapping and relying on mincore() we would have to mlock that memory; we'd want MCL_ONFAULT to be able to test MADV_POPULATE_READ|WRITE; or we'd just want to rely on lseek)

After some digging and testing, I found out that the docs for mincore() are a little outdated, and that
the RFC has an unexpected side effect on the sys call.

The output vector is supposed to set the flag to 1 if the page it indicates was present in either the
page cache or the swap cache. I would like to highlight that if a page got swapped out and reached
swap (i.e. page content no longer stored in the swap cache), the flag gets set to 0.

So indeed, mincore does not report swapped out pages, but AFAIK, it does reports pages which are
still in the swap cache.

There was an attempt to rework mincore altogether and make it retrieve mappings instead [1], but
was quickly reverted [2] because the removed functionality was necessary for some long functioning systems.

On Peter's RFC, it now looks like mincore()'s flags are set to 1 for any shared page that has been
dirtied, whether it is still present, in swap cache or it arrived in swap. AFAIK, only none pages have
their flags set to zero. For private pages, mincore still seems to behave normally.

[1] https://github.com/torvalds/linux/commit/574823bfab82d9d8fa47f422778043fbb4b4f50e
[2] https://github.com/torvalds/linux/commit/30bac164aca750892b93eef350439a0562a68647

> 
>>> 
>>>> 
>>>>> Take CRIU as an example, it has to be correct even if a process would remap a
>>>>> memory region, fork() and unmap in the parent as far as I understand, ...
>>>> 
>>>> Are you talking about dirty bit or swap bit?  I'm a bit confused on why swap
>>>> bit needs to be accurate.  Maybe you mean the dirty bit?
>>> 
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__criu.org_Shared-5Fmemory&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=rRM5dtWOv0DNo5dDxZ2U16jl4WAw6ql5szOKa9cu_RA&m=A5H_4nfdW_jAPHckF-cuCBfRHsm2aij-cr-mELX0uww&s=DZgiYWJgLokyZkBYd5VKOnr5Fbj63aAc01Fu2BPE8Lc&e= 
>>> "Dumping present pages"
>>> 
>>> "... CRIU does not dump all of the data. Instead, it determines which pages
>>> contain it, and only dumps those pages. This is done similarly to how
>>> regular memory dumping and restoring works, i.e. by looking for PRESENT or
>>> SWAPPED bits in owners' pagemap entries."
>>> 
>>> -> Neither PRESENT nor SWAPPED results in memory not getting dumped, which
>>> makes perfect sense.
>>> 
>>> 1) Process A sets up shared memory and writes data to it.
>>> 2) System swaps out memory, hints are setup.
>>> 3) Process A forks Process B, hints are not copied.
>>> 4) Process A unmaps shared memory, hints are dropped.
>>> 5) CRIU migrates process A and B and migrates only PRESENT or SWAPPED in
>>> pagemap.
>>> 6) Process B uses memory in shared memory region. Pages were not migrated.
>>> 
>>> Just one example; feel free to correct me.
>> I think pte marker won't crash criu, what will happen is that it'll see more
>> ptes that used to be none that become the pte markers.  This reminded me that
>> maybe I should teach up mincore() syscall to also be aware of the pte marker at
>> least, and all non_swap_entry() callers.

I think in mincore_pte_range, the call to non_swap_entry(entry) could return true, setting the flag on
the vector to 1 prematurely. Please read my comment above.
> 
> I haven't checked what mincore() is doing, but from what I understand when reading the CRIU doc and the mincore() doc, it does the right thing without requiring any fiddling with pte marker hints. I assume you merely have a performance improvement in mind.
> 
>>> 
>>> 
>>> There is notion of the mincore() systemcall:
>>> 
>>> "There is one particular feature of shared memory dumps worth mentioning.
>>> Sometimes, a shared memory page can exist in the kernel, but it is not
>>> mapped to any process. CRIU detects such pages by calling mincore() on the
>>> shmem segment, which reports back the page in-memory status. The mincore
>>> bitmap is when ANDed with the per-process ones. "
>>> 
>>> Not sure if they actually mean ORed, because otherwise they'd be losing
>>> pages that have been swapped out. "mincore() returns a vector that indicates
>>> whether pages of the calling process's virtual memory are resident in core
>>> (RAM)"
>> I am wildly guessing they ORed the two just because PM_SWAP is not working
>> properly for shmem, so the OR happens only for shmem.  Criu may not only rely
>> on mincore() because they also want the dirty bits.
>> Btw, I noticed in 2016 criu switched from mincore() to lseek():
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_checkpoint-2Drestore_criu_commit_1821acedd04b602b37b587eac5a481094b6274ae&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=rRM5dtWOv0DNo5dDxZ2U16jl4WAw6ql5szOKa9cu_RA&m=A5H_4nfdW_jAPHckF-cuCBfRHsm2aij-cr-mELX0uww&s=kel85AR7AGZnvBymbM7QEwc770HGO2koka-kTiF-r5U&e= 
> 
> Interesting. That's certainly what we want when it comes to skipping holes in files. (before reading that, I wasn't even aware that mincore() existed)
> 
>> Criu should want to know "whether this page has valid data" not "whether this
>> page has swapped out", so lseek() seems to be more suitable, which I'm not
>> aware of before.
> 
> Again, just as you, I learned a lot :)

Same :)
> 
>> I'm now wondering whether for Tiberiu's case mincore() can also be used.  It
>> should just still be a bit slow because it'll look up the cache too, but it
>> should work similarly like the original proposal.

I am afraid that the information returned by mincore is a little too vague to be of better help, compared to what the pagemap should provide in theory. I will have a look to see whether lseek on
proc/map_files works as a "PM_SWAP" equivalent. However, the swap offset would still be missing.
> 
> Very right, maybe we can just avoid tampering with pagemap on shmem completely (which sounds like an excellent idea to me) and document it as "On shared memory, we will never indicate SWAPPED if the pages have been swapped out. Further, PRESENT might be under-indicated: if a shared page is currently not mapped into the page table of a process.". I saw there was a related, proposed doc update, maybe we can finetune that.
> 
We could take into consideration an alternative approach to retrieving the shared page info in user
space, like storing it in sys/fs instead of per process. However, just leaving the pagemap functionality
incomplete, and not providing an alternative to retrieve the missing information, does not seem right. Updating the docs with a "can't do" should be temporary, until an alternative or fix.

Also, I think you are talking about my own doc update patch[3]. If not, please share a link with your
next reply.

[3] https://marc.info/?m=162878395426774
> 
> -- 
> Thanks,
> 
> David / dhildenb
--
Kind regards,

Tibi Georgescu