| Message ID | 9cf5c075-c83f-0915-99ef-b2aa59eca685@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | __HAVE_ARCH_PTE_DEVMAP - bug or intended behaviour? | expand |
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. 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). 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. > Tangentially, is it also right that is_device_{public,private}_page() can > still get non-stub definitions even with CONFIG_DEVICE_{PUBLIC,PRIVATE} > disabled? As it happens, the patch below is enough to dodge the build > failure for my configuration (i.e. CONFIG_FS_DAX && !CONFIG_HMM) by > optimising the offending call away, however I'm not sure I'd want to rely on > that; conceptually, though, it does still seem like it might be appropriate. Yeah seems like a good idea, the combinatorial config options explosion is painful. Can gcc optimize: static inline bool is_device_public_page(const struct page *page) { if (IS_ENABLED(CONFIG_DEVICE_PUBLIC)) return is_zone_device_page(page) && page->pgmap->type == MEMORY_DEVICE_PUBLIC; return false; } That would be nicer than #if/#else/#endif Cheers, Jérôme > Thanks, > Robin. > > ----->8----- > From: Robin Murphy <robin.murphy@arm.com> > Date: Wed, 31 Oct 2018 15:57:17 +0000 > Subject: [PATCH] mm: Clean up is_device_*_page() definitions > > Refactor is_device_{public,private}_page() with is_pci_p2pdma_page() > to make them all consistent in depending on their respective config > options even when CONFIG_DEV_PAGEMAP_OPS is enabled for other reasons. > This allows a little more compile-time optimisation as well as the > conceptual and cosmetic cleanup. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > include/linux/mm.h | 52 ++++++++++++++++++++++------------------------ > 1 file changed, 25 insertions(+), 27 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 1e52b8fd1685..15a49ed5436c 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -879,32 +879,6 @@ static inline bool put_devmap_managed_page(struct page > *page) > } > return false; > } > - > -static inline bool is_device_private_page(const struct page *page) > -{ > - return is_zone_device_page(page) && > - page->pgmap->type == MEMORY_DEVICE_PRIVATE; > -} > - > -static inline bool is_device_public_page(const struct page *page) > -{ > - return is_zone_device_page(page) && > - page->pgmap->type == MEMORY_DEVICE_PUBLIC; > -} > - > -#ifdef CONFIG_PCI_P2PDMA > -static inline bool is_pci_p2pdma_page(const struct page *page) > -{ > - return is_zone_device_page(page) && > - page->pgmap->type == MEMORY_DEVICE_PCI_P2PDMA; > -} > -#else /* CONFIG_PCI_P2PDMA */ > -static inline bool is_pci_p2pdma_page(const struct page *page) > -{ > - return false; > -} > -#endif /* CONFIG_PCI_P2PDMA */ > - > #else /* CONFIG_DEV_PAGEMAP_OPS */ > static inline void dev_pagemap_get_ops(void) > { > @@ -918,22 +892,46 @@ static inline bool put_devmap_managed_page(struct page > *page) > { > return false; > } > +#endif /* CONFIG_DEV_PAGEMAP_OPS */ > > +#if defined(CONFIG_DEV_PAGEMAP_OPS) && defined(CONFIG_DEVICE_PRIVATE) > +static inline bool is_device_private_page(const struct page *page) > +{ > + return is_zone_device_page(page) && > + page->pgmap->type == MEMORY_DEVICE_PRIVATE; > +} > +#else > static inline bool is_device_private_page(const struct page *page) > { > return false; > } > +#endif > > +#if defined(CONFIG_DEV_PAGEMAP_OPS) && defined(CONFIG_DEVICE_PUBLIC) > +static inline bool is_device_public_page(const struct page *page) > +{ > + return is_zone_device_page(page) && > + page->pgmap->type == MEMORY_DEVICE_PUBLIC; > +} > +#else > static inline bool is_device_public_page(const struct page *page) > { > return false; > } > +#endif > > +#if defined(CONFIG_DEV_PAGEMAP_OPS) && defined(CONFIG_PCI_P2PDMA) > +static inline bool is_pci_p2pdma_page(const struct page *page) > +{ > + return is_zone_device_page(page) && > + page->pgmap->type == MEMORY_DEVICE_PCI_P2PDMA; > +} > +#else > static inline bool is_pci_p2pdma_page(const struct page *page) > { > return false; > } > -#endif /* CONFIG_DEV_PAGEMAP_OPS */ > +#endif > > static inline void get_page(struct page *page) > { > -- > 2.19.1.dirty
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.
On Wed, Oct 31, 2018 at 10:08 AM Robin Murphy <robin.murphy@arm.com> 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. It's deliberate, it's only there to allow mm/memory.c to compile. The compiler can see that pfn_t_devmap(pfn) is always false in the !__HAVE_ARCH_PTE_DEVMAP case and throws away the attempt to link to pte_devmap(). The summary is that an architecture needs to devote a free/software pte bit for Linux to indicate "device pfns".
On 31/10/2018 20:41, Dan Williams wrote: > On Wed, Oct 31, 2018 at 10:08 AM Robin Murphy <robin.murphy@arm.com> 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. > > It's deliberate, it's only there to allow mm/memory.c to compile. The > compiler can see that pfn_t_devmap(pfn) is always false in the > !__HAVE_ARCH_PTE_DEVMAP case and throws away the attempt to link to > pte_devmap(). > > The summary is that an architecture needs to devote a free/software > pte bit for Linux to indicate "device pfns". Thanks for the explanation(s), that's been super helpful. So essentially, the WIP configuration I currently have (ARCH_HAS_ZONE_DEVICE=y but !__HAVE_ARCH_PTE_DEVMAP) is fundamentally incomplete, and even if I convince a ZONE_DEVICE=y config to build with the devmap stubs, it would end up going wrong in exciting ways at runtime - is that the gist of it? If that is the case, then I might also have a go at streamlining some of the configs to make those dependencies more apparent. Cheers, Robin.
On Thu, Nov 1, 2018 at 1:10 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 31/10/2018 20:41, Dan Williams wrote: > > On Wed, Oct 31, 2018 at 10:08 AM Robin Murphy <robin.murphy@arm.com> 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. > > > > It's deliberate, it's only there to allow mm/memory.c to compile. The > > compiler can see that pfn_t_devmap(pfn) is always false in the > > !__HAVE_ARCH_PTE_DEVMAP case and throws away the attempt to link to > > pte_devmap(). > > > > The summary is that an architecture needs to devote a free/software > > pte bit for Linux to indicate "device pfns". > > Thanks for the explanation(s), that's been super helpful. So > essentially, the WIP configuration I currently have > (ARCH_HAS_ZONE_DEVICE=y but !__HAVE_ARCH_PTE_DEVMAP) is fundamentally > incomplete, and even if I convince a ZONE_DEVICE=y config to build with > the devmap stubs, it would end up going wrong in exciting ways at > runtime - is that the gist of it? Yes, exactly. > If that is the case, then I might also > have a go at streamlining some of the configs to make those dependencies > more apparent. Sounds good.
diff --git a/include/linux/mm.h b/include/linux/mm.h index 1e52b8fd1685..15a49ed5436c 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -879,32 +879,6 @@ static inline bool put_devmap_managed_page(struct page *page) } return false; } - -static inline bool is_device_private_page(const struct page *page) -{ - return is_zone_device_page(page) && - page->pgmap->type == MEMORY_DEVICE_PRIVATE; -} - -static inline bool is_device_public_page(const struct page *page) -{ - return is_zone_device_page(page) && - page->pgmap->type == MEMORY_DEVICE_PUBLIC; -} - -#ifdef CONFIG_PCI_P2PDMA -static inline bool is_pci_p2pdma_page(const struct page *page) -{ - return is_zone_device_page(page) && - page->pgmap->type == MEMORY_DEVICE_PCI_P2PDMA; -} -#else /* CONFIG_PCI_P2PDMA */ -static inline bool is_pci_p2pdma_page(const struct page *page) -{ - return false; -} -#endif /* CONFIG_PCI_P2PDMA */ - #else /* CONFIG_DEV_PAGEMAP_OPS */ static inline void dev_pagemap_get_ops(void)
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. Tangentially, is it also right that is_device_{public,private}_page() can still get non-stub definitions even with CONFIG_DEVICE_{PUBLIC,PRIVATE} disabled? As it happens, the patch below is enough to dodge the build failure for my configuration (i.e. CONFIG_FS_DAX && !CONFIG_HMM) by optimising the offending call away, however I'm not sure I'd want to rely on that; conceptually, though, it does still seem like it might be appropriate. Thanks, Robin. ----->8----- From: Robin Murphy <robin.murphy@arm.com> Date: Wed, 31 Oct 2018 15:57:17 +0000 Subject: [PATCH] mm: Clean up is_device_*_page() definitions Refactor is_device_{public,private}_page() with is_pci_p2pdma_page() to make them all consistent in depending on their respective config options even when CONFIG_DEV_PAGEMAP_OPS is enabled for other reasons. This allows a little more compile-time optimisation as well as the conceptual and cosmetic cleanup. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- include/linux/mm.h | 52 ++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 27 deletions(-) { @@ -918,22 +892,46 @@ static inline bool put_devmap_managed_page(struct page *page) { return false; } +#endif /* CONFIG_DEV_PAGEMAP_OPS */ +#if defined(CONFIG_DEV_PAGEMAP_OPS) && defined(CONFIG_DEVICE_PRIVATE) +static inline bool is_device_private_page(const struct page *page) +{ + return is_zone_device_page(page) && + page->pgmap->type == MEMORY_DEVICE_PRIVATE; +} +#else static inline bool is_device_private_page(const struct page *page) { return false; } +#endif +#if defined(CONFIG_DEV_PAGEMAP_OPS) && defined(CONFIG_DEVICE_PUBLIC) +static inline bool is_device_public_page(const struct page *page) +{ + return is_zone_device_page(page) && + page->pgmap->type == MEMORY_DEVICE_PUBLIC; +} +#else static inline bool is_device_public_page(const struct page *page) { return false; } +#endif +#if defined(CONFIG_DEV_PAGEMAP_OPS) && defined(CONFIG_PCI_P2PDMA) +static inline bool is_pci_p2pdma_page(const struct page *page) +{ + return is_zone_device_page(page) && + page->pgmap->type == MEMORY_DEVICE_PCI_P2PDMA; +} +#else static inline bool is_pci_p2pdma_page(const struct page *page) { return false; } -#endif /* CONFIG_DEV_PAGEMAP_OPS */ +#endif static inline void get_page(struct page *page) {