| Message ID | 20190221113502.54153-4-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert x86 & arm64 to use generic page walk | expand |
On Thu, Feb 21, 2019 at 11:34:52AM +0000, Steven Price wrote: > From: James Morse <james.morse@arm.com> > > Exposing the pud/pgd levels of the page tables to walk_page_range() means > we may come across the exotic large mappings that come with large areas > of contiguous memory (such as the kernel's linear map). > > For architectures that don't provide p?d_large() macros, provided a > does nothing default. > > Signed-off-by: James Morse <james.morse@arm.com> > Signed-off-by: Steven Price <steven.price@arm.com> > --- > include/asm-generic/pgtable.h | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h > index 05e61e6c843f..f0de24100ac6 100644 > --- a/include/asm-generic/pgtable.h > +++ b/include/asm-generic/pgtable.h > @@ -1186,4 +1186,23 @@ static inline bool arch_has_pfn_modify_check(void) > #define mm_pmd_folded(mm) __is_defined(__PAGETABLE_PMD_FOLDED) > #endif > > +/* > + * p?d_large() - true if this entry is a final mapping to a physical address. It might make sense to s/final/leaf/, but otherwise that's a great definition! > + * This differs from p?d_huge() by the fact that they are always available (if > + * the architecture supports large pages at the appropriate level) even > + * if CONFIG_HUGETLB_PAGE is not defined. I'm not sure if we need this part, since we don't mention p?d_trans_huge(), etc, but either way: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. > + */ > +#ifndef pgd_large > +#define pgd_large(x) 0 > +#endif > +#ifndef p4d_large > +#define p4d_large(x) 0 > +#endif > +#ifndef pud_large > +#define pud_large(x) 0 > +#endif > +#ifndef pmd_large > +#define pmd_large(x) 0 > +#endif > + > #endif /* _ASM_GENERIC_PGTABLE_H */ > -- > 2.20.1 >
On Thu, Feb 21, 2019 at 11:34:52AM +0000, Steven Price wrote: > From: James Morse <james.morse@arm.com> > > Exposing the pud/pgd levels of the page tables to walk_page_range() means > we may come across the exotic large mappings that come with large areas > of contiguous memory (such as the kernel's linear map). > > For architectures that don't provide p?d_large() macros, provided a > does nothing default. Nak, sorry. Power will get broken by the patch. It has pmd_large() inline function, that will be overwritten by the define from this patch. I believe it requires more ground work on arch side in general. All architectures that has huge page support has to provide these helpers (and matching defines) before you can use it in a generic code.
On 21/02/2019 14:28, Kirill A. Shutemov wrote: > On Thu, Feb 21, 2019 at 11:34:52AM +0000, Steven Price wrote: >> From: James Morse <james.morse@arm.com> >> >> Exposing the pud/pgd levels of the page tables to walk_page_range() means >> we may come across the exotic large mappings that come with large areas >> of contiguous memory (such as the kernel's linear map). >> >> For architectures that don't provide p?d_large() macros, provided a >> does nothing default. > > Nak, sorry. > > Power will get broken by the patch. It has pmd_large() inline function, > that will be overwritten by the define from this patch. > > I believe it requires more ground work on arch side in general. > All architectures that has huge page support has to provide these helpers > (and matching defines) before you can use it in a generic code. Sorry about that, I had compile tested on power, but obviously not the right config to actually see the breakage. I'll do some grepping - hopefully this is just a case of exposing the functions/defines that already exist for those architectures. Note that in terms of the new page walking code, these new defines are only used when walking a page table without a VMA (which isn't currently done), so architectures which don't use p?d_large currently will work fine with the generic versions. They only need to provide meaningful definitions when switching to use the walk-without-a-VMA functionality. Thanks for reporting the breakage. Steve
On Thu, Feb 21, 2019 at 02:46:18PM +0000, Steven Price wrote: > On 21/02/2019 14:28, Kirill A. Shutemov wrote: > > On Thu, Feb 21, 2019 at 11:34:52AM +0000, Steven Price wrote: > >> From: James Morse <james.morse@arm.com> > >> > >> Exposing the pud/pgd levels of the page tables to walk_page_range() means > >> we may come across the exotic large mappings that come with large areas > >> of contiguous memory (such as the kernel's linear map). > >> > >> For architectures that don't provide p?d_large() macros, provided a > >> does nothing default. > > > > Nak, sorry. > > > > Power will get broken by the patch. It has pmd_large() inline function, > > that will be overwritten by the define from this patch. > > > > I believe it requires more ground work on arch side in general. > > All architectures that has huge page support has to provide these helpers > > (and matching defines) before you can use it in a generic code. > > Sorry about that, I had compile tested on power, but obviously not the > right config to actually see the breakage. I don't think you'll catch it at compile-time. It would silently override the helper with always-false. > I'll do some grepping - hopefully this is just a case of exposing the > functions/defines that already exist for those architectures. I see the same type of breakage on s390 and sparc. > Note that in terms of the new page walking code, these new defines are > only used when walking a page table without a VMA (which isn't currently > done), so architectures which don't use p?d_large currently will work > fine with the generic versions. They only need to provide meaningful > definitions when switching to use the walk-without-a-VMA functionality. How other architectures would know that they need to provide the helpers to get walk-without-a-VMA functionality? This looks very fragile to me.
On 21/02/2019 14:57, Kirill A. Shutemov wrote: > On Thu, Feb 21, 2019 at 02:46:18PM +0000, Steven Price wrote: >> On 21/02/2019 14:28, Kirill A. Shutemov wrote: >>> On Thu, Feb 21, 2019 at 11:34:52AM +0000, Steven Price wrote: >>>> From: James Morse <james.morse@arm.com> >>>> >>>> Exposing the pud/pgd levels of the page tables to walk_page_range() means >>>> we may come across the exotic large mappings that come with large areas >>>> of contiguous memory (such as the kernel's linear map). >>>> >>>> For architectures that don't provide p?d_large() macros, provided a >>>> does nothing default. >>> >>> Nak, sorry. >>> >>> Power will get broken by the patch. It has pmd_large() inline function, >>> that will be overwritten by the define from this patch. >>> >>> I believe it requires more ground work on arch side in general. >>> All architectures that has huge page support has to provide these helpers >>> (and matching defines) before you can use it in a generic code. >> >> Sorry about that, I had compile tested on power, but obviously not the >> right config to actually see the breakage. > > I don't think you'll catch it at compile-time. It would silently override > the helper with always-false. Ah, that might explain why I missed it. >> I'll do some grepping - hopefully this is just a case of exposing the >> functions/defines that already exist for those architectures. > > I see the same type of breakage on s390 and sparc. > >> Note that in terms of the new page walking code, these new defines are >> only used when walking a page table without a VMA (which isn't currently >> done), so architectures which don't use p?d_large currently will work >> fine with the generic versions. They only need to provide meaningful >> definitions when switching to use the walk-without-a-VMA functionality. > > How other architectures would know that they need to provide the helpers > to get walk-without-a-VMA functionality? This looks very fragile to me. Yes, you've got a good point there. This would apply to the p?d_large macros as well - any arch which (inadvertently) uses the generic version is likely to be fragile/broken. I think probably the best option here is to scrap the generic versions altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which would enable the new functionality to those arches that opt-in. Do you think this would be less fragile? Thanks, Steve
On Thu, Feb 21, 2019 at 05:16:46PM +0000, Steven Price wrote: > >> Note that in terms of the new page walking code, these new defines are > >> only used when walking a page table without a VMA (which isn't currently > >> done), so architectures which don't use p?d_large currently will work > >> fine with the generic versions. They only need to provide meaningful > >> definitions when switching to use the walk-without-a-VMA functionality. > > > > How other architectures would know that they need to provide the helpers > > to get walk-without-a-VMA functionality? This looks very fragile to me. > > Yes, you've got a good point there. This would apply to the p?d_large > macros as well - any arch which (inadvertently) uses the generic version > is likely to be fragile/broken. > > I think probably the best option here is to scrap the generic versions > altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which > would enable the new functionality to those arches that opt-in. Do you > think this would be less fragile? These helpers are useful beyond pagewalker. Can we actually do some grinding and make *all* archs to provide correct helpers? Yes, it's tedious, but not that bad. I think we could provide generic helpers for folded levels in <asm-generic/pgtable-nop?d.h> and rest has to be provided by the arch. Architectures that support only 2 level paging would need to provide pgd_large(), with 3 -- pmd_large() and so on.
On 21/02/2019 21:06, Kirill A. Shutemov wrote: > On Thu, Feb 21, 2019 at 05:16:46PM +0000, Steven Price wrote: >>>> Note that in terms of the new page walking code, these new defines are >>>> only used when walking a page table without a VMA (which isn't currently >>>> done), so architectures which don't use p?d_large currently will work >>>> fine with the generic versions. They only need to provide meaningful >>>> definitions when switching to use the walk-without-a-VMA functionality. >>> >>> How other architectures would know that they need to provide the helpers >>> to get walk-without-a-VMA functionality? This looks very fragile to me. >> >> Yes, you've got a good point there. This would apply to the p?d_large >> macros as well - any arch which (inadvertently) uses the generic version >> is likely to be fragile/broken. >> >> I think probably the best option here is to scrap the generic versions >> altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which >> would enable the new functionality to those arches that opt-in. Do you >> think this would be less fragile? > > These helpers are useful beyond pagewalker. > > Can we actually do some grinding and make *all* archs to provide correct > helpers? Yes, it's tedious, but not that bad. > > I think we could provide generic helpers for folded levels in > <asm-generic/pgtable-nop?d.h> and rest has to be provided by the arch. > Architectures that support only 2 level paging would need to provide > pgd_large(), with 3 -- pmd_large() and so on. Fair enough, I'll have a go and hopefully people will be able to correct it if I make any mistakes - I'm certainly not going to be able to test all architectures myself. Steve
Hi Kirill, On Thu, Feb 21, 2019 at 05:57:06PM +0300, Kirill A. Shutemov wrote: > On Thu, Feb 21, 2019 at 02:46:18PM +0000, Steven Price wrote: > > On 21/02/2019 14:28, Kirill A. Shutemov wrote: > > > On Thu, Feb 21, 2019 at 11:34:52AM +0000, Steven Price wrote: > > >> From: James Morse <james.morse@arm.com> > > >> > > >> Exposing the pud/pgd levels of the page tables to walk_page_range() means > > >> we may come across the exotic large mappings that come with large areas > > >> of contiguous memory (such as the kernel's linear map). > > >> > > >> For architectures that don't provide p?d_large() macros, provided a > > >> does nothing default. > > > > > > Nak, sorry. > > > > > > Power will get broken by the patch. It has pmd_large() inline function, > > > that will be overwritten by the define from this patch. > > > > > > I believe it requires more ground work on arch side in general. > > > All architectures that has huge page support has to provide these helpers > > > (and matching defines) before you can use it in a generic code. > > > > Sorry about that, I had compile tested on power, but obviously not the > > right config to actually see the breakage. > > I don't think you'll catch it at compile-time. It would silently override > the helper with always-false. Can you explain why the compiler would override the helper define in, e.g. arch/powerpc/include/asm/pgtable.h with the generic (0)? Actually, I've tried to compile this on power with deliberately adding errors to both power-specific and the generic definition of pmd_large and the compilation failed the way I expected in the power-specific helper. > > I'll do some grepping - hopefully this is just a case of exposing the > > functions/defines that already exist for those architectures. > > I see the same type of breakage on s390 and sparc. > > > Note that in terms of the new page walking code, these new defines are > > only used when walking a page table without a VMA (which isn't currently > > done), so architectures which don't use p?d_large currently will work > > fine with the generic versions. They only need to provide meaningful > > definitions when switching to use the walk-without-a-VMA functionality. > > How other architectures would know that they need to provide the helpers > to get walk-without-a-VMA functionality? This looks very fragile to me. > > -- > Kirill A. Shutemov
Him Kirill, On Fri, Feb 22, 2019 at 12:06:18AM +0300, Kirill A. Shutemov wrote: > On Thu, Feb 21, 2019 at 05:16:46PM +0000, Steven Price wrote: > > >> Note that in terms of the new page walking code, these new defines are > > >> only used when walking a page table without a VMA (which isn't currently > > >> done), so architectures which don't use p?d_large currently will work > > >> fine with the generic versions. They only need to provide meaningful > > >> definitions when switching to use the walk-without-a-VMA functionality. > > > > > > How other architectures would know that they need to provide the helpers > > > to get walk-without-a-VMA functionality? This looks very fragile to me. > > > > Yes, you've got a good point there. This would apply to the p?d_large > > macros as well - any arch which (inadvertently) uses the generic version > > is likely to be fragile/broken. > > > > I think probably the best option here is to scrap the generic versions > > altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which > > would enable the new functionality to those arches that opt-in. Do you > > think this would be less fragile? > > These helpers are useful beyond pagewalker. > > Can we actually do some grinding and make *all* archs to provide correct > helpers? Yes, it's tedious, but not that bad. Many architectures simply cannot support non-leaf entries at the higher levels. I think letting the use a generic helper actually does make sense. > I think we could provide generic helpers for folded levels in > <asm-generic/pgtable-nop?d.h> and rest has to be provided by the arch. > Architectures that support only 2 level paging would need to provide > pgd_large(), with 3 -- pmd_large() and so on. > > -- > Kirill A. Shutemov
On Fri, Mar 01, 2019 at 01:49:53PM +0200, Mike Rapoport wrote: > Hi Kirill, > > On Thu, Feb 21, 2019 at 05:57:06PM +0300, Kirill A. Shutemov wrote: > > On Thu, Feb 21, 2019 at 02:46:18PM +0000, Steven Price wrote: > > > On 21/02/2019 14:28, Kirill A. Shutemov wrote: > > > > On Thu, Feb 21, 2019 at 11:34:52AM +0000, Steven Price wrote: > > > >> From: James Morse <james.morse@arm.com> > > > >> > > > >> Exposing the pud/pgd levels of the page tables to walk_page_range() means > > > >> we may come across the exotic large mappings that come with large areas > > > >> of contiguous memory (such as the kernel's linear map). > > > >> > > > >> For architectures that don't provide p?d_large() macros, provided a > > > >> does nothing default. > > > > > > > > Nak, sorry. > > > > > > > > Power will get broken by the patch. It has pmd_large() inline function, > > > > that will be overwritten by the define from this patch. > > > > > > > > I believe it requires more ground work on arch side in general. > > > > All architectures that has huge page support has to provide these helpers > > > > (and matching defines) before you can use it in a generic code. > > > > > > Sorry about that, I had compile tested on power, but obviously not the > > > right config to actually see the breakage. > > > > I don't think you'll catch it at compile-time. It would silently override > > the helper with always-false. > > Can you explain why the compiler would override the helper define in, e.g. > arch/powerpc/include/asm/pgtable.h with the generic (0)? This one will not be overrided, but the other one will. See arch/powerpc/include/asm/book3s/64/pgtable.h
On Fri, Mar 01, 2019 at 01:53:01PM +0200, Mike Rapoport wrote: > Him Kirill, > > On Fri, Feb 22, 2019 at 12:06:18AM +0300, Kirill A. Shutemov wrote: > > On Thu, Feb 21, 2019 at 05:16:46PM +0000, Steven Price wrote: > > > >> Note that in terms of the new page walking code, these new defines are > > > >> only used when walking a page table without a VMA (which isn't currently > > > >> done), so architectures which don't use p?d_large currently will work > > > >> fine with the generic versions. They only need to provide meaningful > > > >> definitions when switching to use the walk-without-a-VMA functionality. > > > > > > > > How other architectures would know that they need to provide the helpers > > > > to get walk-without-a-VMA functionality? This looks very fragile to me. > > > > > > Yes, you've got a good point there. This would apply to the p?d_large > > > macros as well - any arch which (inadvertently) uses the generic version > > > is likely to be fragile/broken. > > > > > > I think probably the best option here is to scrap the generic versions > > > altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which > > > would enable the new functionality to those arches that opt-in. Do you > > > think this would be less fragile? > > > > These helpers are useful beyond pagewalker. > > > > Can we actually do some grinding and make *all* archs to provide correct > > helpers? Yes, it's tedious, but not that bad. > > Many architectures simply cannot support non-leaf entries at the higher > levels. I think letting the use a generic helper actually does make sense. I disagree. It's makes sense if the level doesn't exists on the arch. But if the level exists, it will be less frugile to ask the arch to provide the helper. Even if it is dummy always-false.
On 01/03/2019 12:30, Kirill A. Shutemov wrote: > On Fri, Mar 01, 2019 at 01:53:01PM +0200, Mike Rapoport wrote: >> Him Kirill, >> >> On Fri, Feb 22, 2019 at 12:06:18AM +0300, Kirill A. Shutemov wrote: >>> On Thu, Feb 21, 2019 at 05:16:46PM +0000, Steven Price wrote: >>>>>> Note that in terms of the new page walking code, these new defines are >>>>>> only used when walking a page table without a VMA (which isn't currently >>>>>> done), so architectures which don't use p?d_large currently will work >>>>>> fine with the generic versions. They only need to provide meaningful >>>>>> definitions when switching to use the walk-without-a-VMA functionality. >>>>> >>>>> How other architectures would know that they need to provide the helpers >>>>> to get walk-without-a-VMA functionality? This looks very fragile to me. >>>> >>>> Yes, you've got a good point there. This would apply to the p?d_large >>>> macros as well - any arch which (inadvertently) uses the generic version >>>> is likely to be fragile/broken. >>>> >>>> I think probably the best option here is to scrap the generic versions >>>> altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which >>>> would enable the new functionality to those arches that opt-in. Do you >>>> think this would be less fragile? >>> >>> These helpers are useful beyond pagewalker. >>> >>> Can we actually do some grinding and make *all* archs to provide correct >>> helpers? Yes, it's tedious, but not that bad. >> >> Many architectures simply cannot support non-leaf entries at the higher >> levels. I think letting the use a generic helper actually does make sense. > > I disagree. > > It's makes sense if the level doesn't exists on the arch. This is what patch 24 [1] of the series does - if the level doesn't exist then appropriate stubs are provided. > But if the level exists, it will be less frugile to ask the arch to > provide the helper. Even if it is dummy always-false. The problem (as I see it), is we need a reliable set of p?d_large() implementations to be able to walk arbitrary page tables. Either the entire functionality of walking page tables without a VMA has to be an opt-in per architecture, or we need to mandate that every architecture provide these implementations. I could provide an asm-generic header to provide a complete set of dummy implementations for architectures that don't support large pages at all, but that seems a bit overkill when most architectures only need to define 2 or 3 implementations (the rest being provided by the folded-levels automatically). Thanks, Steve [1] https://lore.kernel.org/lkml/20190227170608.27963-25-steven.price@arm.com/
On Fri, Mar 01, 2019 at 01:39:30PM +0000, Steven Price wrote: > On 01/03/2019 12:30, Kirill A. Shutemov wrote: > > On Fri, Mar 01, 2019 at 01:53:01PM +0200, Mike Rapoport wrote: > >> Him Kirill, > >> > >> On Fri, Feb 22, 2019 at 12:06:18AM +0300, Kirill A. Shutemov wrote: > >>> On Thu, Feb 21, 2019 at 05:16:46PM +0000, Steven Price wrote: > >>>>>> Note that in terms of the new page walking code, these new defines are > >>>>>> only used when walking a page table without a VMA (which isn't currently > >>>>>> done), so architectures which don't use p?d_large currently will work > >>>>>> fine with the generic versions. They only need to provide meaningful > >>>>>> definitions when switching to use the walk-without-a-VMA functionality. > >>>>> > >>>>> How other architectures would know that they need to provide the helpers > >>>>> to get walk-without-a-VMA functionality? This looks very fragile to me. > >>>> > >>>> Yes, you've got a good point there. This would apply to the p?d_large > >>>> macros as well - any arch which (inadvertently) uses the generic version > >>>> is likely to be fragile/broken. > >>>> > >>>> I think probably the best option here is to scrap the generic versions > >>>> altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which > >>>> would enable the new functionality to those arches that opt-in. Do you > >>>> think this would be less fragile? > >>> > >>> These helpers are useful beyond pagewalker. > >>> > >>> Can we actually do some grinding and make *all* archs to provide correct > >>> helpers? Yes, it's tedious, but not that bad. > >> > >> Many architectures simply cannot support non-leaf entries at the higher > >> levels. I think letting the use a generic helper actually does make sense. > > > > I disagree. > > > > It's makes sense if the level doesn't exists on the arch. > > This is what patch 24 [1] of the series does - if the level doesn't > exist then appropriate stubs are provided. > > > But if the level exists, it will be less frugile to ask the arch to > > provide the helper. Even if it is dummy always-false. > > The problem (as I see it), is we need a reliable set of p?d_large() > implementations to be able to walk arbitrary page tables. Either the > entire functionality of walking page tables without a VMA has to be an > opt-in per architecture, or we need to mandate that every architecture > provide these implementations. I agree that we need a reliable set of p?d_large(), but I'm still not convinced that every architecture should provide these. Why having generic versions if p?d_large() is more fragile, than e.g. p??__access_permitted() or atomic ops? IMHO, adding those functions/macros for architectures that support large pages and providing defines to avoid override of 'static inline' implementations would be robust enough and will avoid unnecessary stubs in architectures that don't have large pages. > I could provide an asm-generic header to provide a complete set of dummy > implementations for architectures that don't support large pages at all, > but that seems a bit overkill when most architectures only need to > define 2 or 3 implementations (the rest being provided by the > folded-levels automatically). > > Thanks, > > Steve > > [1] > https://lore.kernel.org/lkml/20190227170608.27963-25-steven.price@arm.com/
On 03/03/2019 07:12, Mike Rapoport wrote: > On Fri, Mar 01, 2019 at 01:39:30PM +0000, Steven Price wrote: >> On 01/03/2019 12:30, Kirill A. Shutemov wrote: >>> On Fri, Mar 01, 2019 at 01:53:01PM +0200, Mike Rapoport wrote: >>>> Him Kirill, >>>> >>>> On Fri, Feb 22, 2019 at 12:06:18AM +0300, Kirill A. Shutemov wrote: >>>>> On Thu, Feb 21, 2019 at 05:16:46PM +0000, Steven Price wrote: >>>>>>>> Note that in terms of the new page walking code, these new defines are >>>>>>>> only used when walking a page table without a VMA (which isn't currently >>>>>>>> done), so architectures which don't use p?d_large currently will work >>>>>>>> fine with the generic versions. They only need to provide meaningful >>>>>>>> definitions when switching to use the walk-without-a-VMA functionality. >>>>>>> >>>>>>> How other architectures would know that they need to provide the helpers >>>>>>> to get walk-without-a-VMA functionality? This looks very fragile to me. >>>>>> >>>>>> Yes, you've got a good point there. This would apply to the p?d_large >>>>>> macros as well - any arch which (inadvertently) uses the generic version >>>>>> is likely to be fragile/broken. >>>>>> >>>>>> I think probably the best option here is to scrap the generic versions >>>>>> altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which >>>>>> would enable the new functionality to those arches that opt-in. Do you >>>>>> think this would be less fragile? >>>>> >>>>> These helpers are useful beyond pagewalker. >>>>> >>>>> Can we actually do some grinding and make *all* archs to provide correct >>>>> helpers? Yes, it's tedious, but not that bad. >>>> >>>> Many architectures simply cannot support non-leaf entries at the higher >>>> levels. I think letting the use a generic helper actually does make sense. >>> >>> I disagree. >>> >>> It's makes sense if the level doesn't exists on the arch. >> >> This is what patch 24 [1] of the series does - if the level doesn't >> exist then appropriate stubs are provided. >> >>> But if the level exists, it will be less frugile to ask the arch to >>> provide the helper. Even if it is dummy always-false. >> >> The problem (as I see it), is we need a reliable set of p?d_large() >> implementations to be able to walk arbitrary page tables. Either the >> entire functionality of walking page tables without a VMA has to be an >> opt-in per architecture, or we need to mandate that every architecture >> provide these implementations. > > I agree that we need a reliable set of p?d_large(), but I'm still not > convinced that every architecture should provide these. > > Why having generic versions if p?d_large() is more fragile, than e.g. > p??__access_permitted() or atomic ops? Personally I feel having p?d_large implemented for each arch has the following benefits: * Matches p?d_present/p?d_none/p?d_bad which all similarly have to be implemented for all arches except for folded levels (when folded using the generic code). * Gives the architecture maintainers a heads-up and an opportunity to ensure that the implementations I've written are correct rather than silently picking up the generic version. * When adding a new architecture it will be obvious that p?d_large implementations are needed. The benefits of having a generic version seem to be: * No boiler plate for the architectures that don't support large pages (saves a handful of lines). * Easier to merge (fewer patches). While the last one is certainly appealing (to me at least), I'm not convinced the benefits of the generic version outweigh those of providing implementations per-arch. Am I missing something? > IMHO, adding those functions/macros for architectures that support large > pages and providing defines to avoid override of 'static inline' implementations > would be robust enough and will avoid unnecessary stubs in architectures > that don't have large pages. Clearly at run time there's no difference in the "robustness" - the code generation should be the same. So it's purely down to development processes. However, if you prefer I can resurrect the generic versions and drop the patches that simply add dummy implementations. Steve
On Mon, Mar 04, 2019 at 02:35:56PM +0000, Steven Price wrote: > On 03/03/2019 07:12, Mike Rapoport wrote: > > On Fri, Mar 01, 2019 at 01:39:30PM +0000, Steven Price wrote: > >> On 01/03/2019 12:30, Kirill A. Shutemov wrote: > >>> On Fri, Mar 01, 2019 at 01:53:01PM +0200, Mike Rapoport wrote: > >>>> Him Kirill, > >>>> > >>>> On Fri, Feb 22, 2019 at 12:06:18AM +0300, Kirill A. Shutemov wrote: > >>>>> On Thu, Feb 21, 2019 at 05:16:46PM +0000, Steven Price wrote: > >>>>>>>> Note that in terms of the new page walking code, these new defines are > >>>>>>>> only used when walking a page table without a VMA (which isn't currently > >>>>>>>> done), so architectures which don't use p?d_large currently will work > >>>>>>>> fine with the generic versions. They only need to provide meaningful > >>>>>>>> definitions when switching to use the walk-without-a-VMA functionality. > >>>>>>> > >>>>>>> How other architectures would know that they need to provide the helpers > >>>>>>> to get walk-without-a-VMA functionality? This looks very fragile to me. > >>>>>> > >>>>>> Yes, you've got a good point there. This would apply to the p?d_large > >>>>>> macros as well - any arch which (inadvertently) uses the generic version > >>>>>> is likely to be fragile/broken. > >>>>>> > >>>>>> I think probably the best option here is to scrap the generic versions > >>>>>> altogether and simply introduce a ARCH_HAS_PXD_LARGE config option which > >>>>>> would enable the new functionality to those arches that opt-in. Do you > >>>>>> think this would be less fragile? > >>>>> > >>>>> These helpers are useful beyond pagewalker. > >>>>> > >>>>> Can we actually do some grinding and make *all* archs to provide correct > >>>>> helpers? Yes, it's tedious, but not that bad. > >>>> > >>>> Many architectures simply cannot support non-leaf entries at the higher > >>>> levels. I think letting the use a generic helper actually does make sense. > >>> > >>> I disagree. > >>> > >>> It's makes sense if the level doesn't exists on the arch. > >> > >> This is what patch 24 [1] of the series does - if the level doesn't > >> exist then appropriate stubs are provided. > >> > >>> But if the level exists, it will be less frugile to ask the arch to > >>> provide the helper. Even if it is dummy always-false. > >> > >> The problem (as I see it), is we need a reliable set of p?d_large() > >> implementations to be able to walk arbitrary page tables. Either the > >> entire functionality of walking page tables without a VMA has to be an > >> opt-in per architecture, or we need to mandate that every architecture > >> provide these implementations. > > > > I agree that we need a reliable set of p?d_large(), but I'm still not > > convinced that every architecture should provide these. > > > > Why having generic versions if p?d_large() is more fragile, than e.g. > > p??__access_permitted() or atomic ops? > > Personally I feel having p?d_large implemented for each arch has the > following benefits: > > * Matches p?d_present/p?d_none/p?d_bad which all similarly have to be > implemented for all arches except for folded levels (when folded using > the generic code). > > * Gives the architecture maintainers a heads-up and an opportunity to > ensure that the implementations I've written are correct rather than > silently picking up the generic version. > > * When adding a new architecture it will be obvious that p?d_large > implementations are needed. > > The benefits of having a generic version seem to be: > > * No boiler plate for the architectures that don't support large pages > (saves a handful of lines). > > * Easier to merge (fewer patches). > > While the last one is certainly appealing (to me at least), I'm not > convinced the benefits of the generic version outweigh those of > providing implementations per-arch. > > Am I missing something? > > > IMHO, adding those functions/macros for architectures that support large > > pages and providing defines to avoid override of 'static inline' implementations > > would be robust enough and will avoid unnecessary stubs in architectures > > that don't have large pages. > > Clearly at run time there's no difference in the "robustness" - the code > generation should be the same. So it's purely down to development processes. > > However, if you prefer I can resurrect the generic versions and drop the > patches that simply add dummy implementations. My concern was the code duplication, which didn't seem necessary. It's not only about saving a handful of lines, but rather having as many of the code shared by different architectures actually shared and not copied. I'd really appreciate having the dummy versions in include/asm-generic rather than all over arch/*/include/asm. > Steve >
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h index 05e61e6c843f..f0de24100ac6 100644 --- a/include/asm-generic/pgtable.h +++ b/include/asm-generic/pgtable.h @@ -1186,4 +1186,23 @@ static inline bool arch_has_pfn_modify_check(void) #define mm_pmd_folded(mm) __is_defined(__PAGETABLE_PMD_FOLDED) #endif +/* + * p?d_large() - true if this entry is a final mapping to a physical address. + * This differs from p?d_huge() by the fact that they are always available (if + * the architecture supports large pages at the appropriate level) even + * if CONFIG_HUGETLB_PAGE is not defined. + */ +#ifndef pgd_large +#define pgd_large(x) 0 +#endif +#ifndef p4d_large +#define p4d_large(x) 0 +#endif +#ifndef pud_large +#define pud_large(x) 0 +#endif +#ifndef pmd_large +#define pmd_large(x) 0 +#endif + #endif /* _ASM_GENERIC_PGTABLE_H */