Re: __HAVE_ARCH_PTE_DEVMAP - bug or intended behaviour?

[Dan Williams <dan.j.williams@intel.com>]

On Wed, Oct 31, 2018 at 12:00 PM Jerome Glisse <jglisse@redhat.com> wrote:
>
> On Wed, Oct 31, 2018 at 05:08:23PM +0000, Robin Murphy wrote:
> > Hi mm folks,
> >
> > I'm looking at ZONE_DEVICE support for arm64, and trying to make sense of a
> > build failure has led me down the rabbit hole of pfn_t.h, and specifically
> > __HAVE_ARCH_PTE_DEVMAP in this first instance.
> >
> > The failure itself is a link error in remove_migration_pte() due to a
> > missing definition of pte_mkdevmap(), but I'm a little confused at the fact
> > that it's explicitly declared without a definition, as if that breakage is
> > deliberate.
> >
> > So, is the !__HAVE_ARCH_PTE_DEVMAP case actually expected to work? If not,
> > then it seems to me that the relevant code could just be gated by
> > CONFIG_ZONE_DEVICE directly to remove the confusion. If it is, though, then
> > what should the generic definitions of p??_mkdevmap() be? I guess either way
> > I still need to figure out the implications of _PAGE_DEVMAP at the arch end
> > and whether/how arm64 should implement it, but given this initial hurdle
> > it's not clear exactly where to go next.
>
> AFAIR you can get ZONE_DEVICE without PTE_DEVMAP, PTE_DEVMAP is an
> optimization for pte_devmap() test ie being able to only have to
> look at pte value to determine if it is a pte pointing to a ZONE_DEVICE
> page versus needing to lookup the struct page.

No, it's not an optimization it's required for get_user_pages(). The
gup path uses the PTE_DEVMAP flag to determine that it needs to first
pin a device hosting the pfn (get_dev_pagemap()), before pinning any
associated pages. This allows device teardown operations to coordinate
with in-flight gup requests.

> As all architecture so far all have PTE_DEVMAP it might very well
> be that the !_HAVE_ARCH_PTE_DEVMAP case is broken (either from the
> start or because of changes made since it was added). It kind of
> looks broken at least when i glance at it now (ie the default
> pte_devmap() should lookup struct page and check if it is a ZONE
> DEVICE page).

That's the wrong way round because the 'struct page' object could be
freed at any time if you don't have a dev_pagemap() reference. So,
ZONE_DEVICE requires P??_DEVMAP.

> So your life will be easier if you can do __HAVE_ARCH_PTE_DEVMAP
> as you will not need to debug the !__HAVE_ARCH_PTE_DEVMAP case.

Per above, no.