diff mbox series

[v2,03/13] mm: Add generic p?d_large() macros

Message ID 20190221113502.54153-4-steven.price@arm.com
State New, archived
Headers show
Series Convert x86 & arm64 to use generic page walk | expand

Commit Message

Steven Price Feb. 21, 2019, 11:34 a.m. UTC
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(+)

Comments

Mark Rutland Feb. 21, 2019, 1:41 p.m. UTC | #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.
> 
> 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
>
Kirill A. Shutemov Feb. 21, 2019, 2:28 p.m. UTC | #2
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.
Steven Price Feb. 21, 2019, 2:46 p.m. UTC | #3
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
Kirill A. Shutemov Feb. 21, 2019, 2:57 p.m. UTC | #4
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.
Steven Price Feb. 21, 2019, 5:16 p.m. UTC | #5
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
Kirill A. Shutemov Feb. 21, 2019, 9:06 p.m. UTC | #6
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.
Steven Price Feb. 22, 2019, 10:21 a.m. UTC | #7
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
Mike Rapoport March 1, 2019, 11:49 a.m. UTC | #8
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
Mike Rapoport March 1, 2019, 11:53 a.m. UTC | #9
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
Kirill A. Shutemov March 1, 2019, 12:28 p.m. UTC | #10
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
Kirill A. Shutemov March 1, 2019, 12:30 p.m. UTC | #11
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.
Steven Price March 1, 2019, 1:39 p.m. UTC | #12
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/
Mike Rapoport March 3, 2019, 7:12 a.m. UTC | #13
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/
Steven Price March 4, 2019, 2:35 p.m. UTC | #14
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
Mike Rapoport March 4, 2019, 2:53 p.m. UTC | #15
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 mbox series

Patch

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 */