Re: [PATCH v1 12/12] mm/rmap: keep mapcount untouched for device-exclusive entries

[David Hildenbrand]

> > Assume you have a THP (or any mTHP today). You can easily trigger the
> > scenario that folio_mapcount() != 0 with active device-exclusive entries,
> > and you start doing rmap walks and stumble over these device-exclusive
> > entries and *not* handle them properly. Note that more and more systems are
> > configured to just give you THP unless you explicitly opted-out using
> > MADV_NOHUGEPAGE early.
> >
> > Note that b756a3b5e7ea added that hunk that still walks these
> > device-exclusive entries in rmap code, but didn't actually update the rmap
> > walkers:
> >
> > @@ -102,7 +104,8 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw)
> >
> >                  /* Handle un-addressable ZONE_DEVICE memory */
> >                  entry = pte_to_swp_entry(*pvmw->pte);
> > -               if (!is_device_private_entry(entry))
> > +               if (!is_device_private_entry(entry) &&
> > +                   !is_device_exclusive_entry(entry))
> >                          return false;
> >
> >                  pfn = swp_offset(entry);
> >
> > That was the right thing to do, because they resemble PROT_NONE entries and
> > not migration entries or anything else that doesn't hold a folio reference).
>
> Yeah I got that part. What I meant is that doubling down on this needs a
> full audit and cannot rely on "we already have device private entries
> going through these paths for much longer", which was the impression I
> got. I guess it worked, thanks for doing that below :-)

I know I know, I shouldn't have touched it ... :)

So yeah, I'll spend some extra work on sorting out the other cases.

> And at least from my very rough understanding of mm, at least around all
> this gpu stuff, tracking device exclusive mappings like real cpu mappings
> makes sense, they do indeed act like PROT_NONE with some magic to restore
> access on fault.
>
> I do wonder a bit though what else is all not properly tracked because
> they should be like prot_none except arent. I guess we'll find those as we
> hit them :-/

Likely a lot of stuff. But more in a "entry gets ignored -- 
functionality not implemented, move along" way, because all page table 
walkers have to care about !pte_present() already; it's just RMAP code 
that so far never required it.

[...]

> > If thp constantly reassembles a pmd entry because hey all the
> > > memory is contig and userspace allocated a chunk of memory to place
> > > atomics that alternate between cpu and gpu nicely separated by 4k pages,
> > > then we'll thrash around invalidating ptes to no end. So might be more
> > > fallout here.
> >
> > khugepaged will back off once it sees an exclusive entry, so collapsing
> > could only happen once everything is non-exclusive. See
> > __collapse_huge_page_isolate() as an example.
>
> Ah ok. I think might be good to add that to the commit message, so that
> people who don't understand mm deeply (like me) aren't worried when they
> stumble over this change in the future again when digging around.

Will do, thanks for raising that concern!

> > It's really only page_vma_mapped_walk() callers that are affected by this
> > change, not any other page table walkers.
>
> I guess my mm understanding is just not up to that, but I couldn't figure
> out why just looking at page_vma_mapped_walk() only is good enough?

See above: these never had to handle !page_present() before -- in 
contrast to the other page table walkers.

So nothing bad happens when these page table walkers traverse these 
PTEs, it's just that the functionality will usually be implemented.

Take MADV_PAGEOUT as an example: madvise_cold_or_pageout_pte_range() 
will simply skip "!pte_present()", because it wouldn't know what to do 
in that case.

Of course, there could be page table walkers that check all cases and 
bail out if they find something unexpected: do_swap_page() cannot make 
forward progress and will inject a VM_FAULT_SIGBUS if it doesn't 
recognize the entry. But these are rather rare.

We could enlighten selected page table walkers to handle 
device-exclusive where it really makes sense later.

--
Cheers,

David / dhildenb