| Message ID | 20190722154210.42799-11-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generic page walk and ptdump | expand |
On Mon, Jul 22, 2019 at 04:41:59PM +0100, Steven Price wrote: > 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 all p?d_leaf() macros, provide > generic do nothing default that are suitable where there cannot be leaf > pages that that level. > > Signed-off-by: Steven Price <steven.price@arm.com> Not a big deal, but it would probably make sense for this to be patch 1 in the series, given it defines the semantic of p?d_leaf(), and they're not used until we provide all the architectural implemetnations anyway. It might also be worth pointing out the reasons for this naming, e.g. p?d_large() aren't currently generic, and this name minimizes potential confusion between p?d_{large,huge}(). > --- > 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 75d9d68a6de7..46275896ca66 100644 > --- a/include/asm-generic/pgtable.h > +++ b/include/asm-generic/pgtable.h > @@ -1188,4 +1188,23 @@ static inline bool arch_has_pfn_modify_check(void) > #define mm_pmd_folded(mm) __is_defined(__PAGETABLE_PMD_FOLDED) > #endif > > +/* > + * p?d_leaf() - 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. > + */ I assume it's only safe to call these on valid entries? I think it would be worth calling that out explicitly. Otherwise, this looks sound to me: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. > +#ifndef pgd_leaf > +#define pgd_leaf(x) 0 > +#endif > +#ifndef p4d_leaf > +#define p4d_leaf(x) 0 > +#endif > +#ifndef pud_leaf > +#define pud_leaf(x) 0 > +#endif > +#ifndef pmd_leaf > +#define pmd_leaf(x) 0 > +#endif > + > #endif /* _ASM_GENERIC_PGTABLE_H */ > -- > 2.20.1 >
On 23/07/2019 10:41, Mark Rutland wrote: > On Mon, Jul 22, 2019 at 04:41:59PM +0100, Steven Price wrote: >> 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 all p?d_leaf() macros, provide >> generic do nothing default that are suitable where there cannot be leaf >> pages that that level. >> >> Signed-off-by: Steven Price <steven.price@arm.com> > > Not a big deal, but it would probably make sense for this to be patch 1 > in the series, given it defines the semantic of p?d_leaf(), and they're > not used until we provide all the architectural implemetnations anyway. Sure, I'll move it. When it was named p?d_large() this had to come after some architectures that implement p?d_large() as static inline. But p?d_leaf() doesn't have that issue. > It might also be worth pointing out the reasons for this naming, e.g. > p?d_large() aren't currently generic, and this name minimizes potential > confusion between p?d_{large,huge}(). Ok, how about: The name p?d_leaf() is chosen because to minimize the confusion with existing uses of "large" pages and "huge" pages which do not necessary mean that the entry is a leaf (for example it may be a set of contiguous entries that only take 1 TLB slot). For the purpose of walking the page tables we don't need to know how it will be represented in the TLB, but we do need to know for sure if it is a leaf of the tree. >> --- >> 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 75d9d68a6de7..46275896ca66 100644 >> --- a/include/asm-generic/pgtable.h >> +++ b/include/asm-generic/pgtable.h >> @@ -1188,4 +1188,23 @@ static inline bool arch_has_pfn_modify_check(void) >> #define mm_pmd_folded(mm) __is_defined(__PAGETABLE_PMD_FOLDED) >> #endif >> >> +/* >> + * p?d_leaf() - 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. >> + */ > > I assume it's only safe to call these on valid entries? I think it would > be worth calling that out explicitly. Yes only meaningful on valid entries - I'll add that as a comment. > Otherwise, this looks sound to me: > > Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks for the review Steve
On 07/23/2019 03:11 PM, Mark Rutland wrote: > On Mon, Jul 22, 2019 at 04:41:59PM +0100, Steven Price wrote: >> 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 all p?d_leaf() macros, provide >> generic do nothing default that are suitable where there cannot be leaf >> pages that that level. >> >> Signed-off-by: Steven Price <steven.price@arm.com> > > Not a big deal, but it would probably make sense for this to be patch 1 > in the series, given it defines the semantic of p?d_leaf(), and they're > not used until we provide all the architectural implemetnations anyway. Agreed. > > It might also be worth pointing out the reasons for this naming, e.g. > p?d_large() aren't currently generic, and this name minimizes potential > confusion between p?d_{large,huge}(). Agreed. But these fallback also need to first check non-availability of large pages. I am not sure whether CONFIG_HUGETLB_PAGE config being clear indicates that conclusively or not. Being a page table leaf entry has a broader meaning than a large page but that is really not the case today. All leaf entries here are large page entries from MMU perspective. This dependency can definitely be removed when there are other types of leaf entries but for now IMHO it feels bit problematic not to directly associate leaf entries with large pages in config restriction while doing exactly the same.
On 28/07/2019 12:44, Anshuman Khandual wrote: > > > On 07/23/2019 03:11 PM, Mark Rutland wrote: >> On Mon, Jul 22, 2019 at 04:41:59PM +0100, Steven Price wrote: >>> 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 all p?d_leaf() macros, provide >>> generic do nothing default that are suitable where there cannot be leaf >>> pages that that level. >>> >>> Signed-off-by: Steven Price <steven.price@arm.com> >> >> Not a big deal, but it would probably make sense for this to be patch 1 >> in the series, given it defines the semantic of p?d_leaf(), and they're >> not used until we provide all the architectural implemetnations anyway. > > Agreed. > >> >> It might also be worth pointing out the reasons for this naming, e.g. >> p?d_large() aren't currently generic, and this name minimizes potential >> confusion between p?d_{large,huge}(). > > Agreed. But these fallback also need to first check non-availability of large > pages. I am not sure whether CONFIG_HUGETLB_PAGE config being clear indicates > that conclusively or not. Being a page table leaf entry has a broader meaning > than a large page but that is really not the case today. All leaf entries here > are large page entries from MMU perspective. This dependency can definitely be > removed when there are other types of leaf entries but for now IMHO it feels > bit problematic not to directly associate leaf entries with large pages in > config restriction while doing exactly the same. The intention here is that the page walkers are able to walk any type of page table entry which the kernel may use. CONFIG_HUGETLB_PAGE only controls whether "huge TLB pages" are used by user space processes. It's quite possible that option to not be selected but the linear mapping to have been mapped using "large pages" (i.e. leaf entries further up the tree than normal). One of the goals was to avoid tying the new functions to a configuration option but instead match the hardware architecture. Of course this isn't possible in the most general case (e.g. an architecture may have multiple hardware page table formats). But to the extent that other functions like p?d_none() work the desire is that p?d_leaf() should also work. Steve
On Sun, Jul 28, 2019 at 05:14:31PM +0530, Anshuman Khandual wrote: > On 07/23/2019 03:11 PM, Mark Rutland wrote: > > It might also be worth pointing out the reasons for this naming, e.g. > > p?d_large() aren't currently generic, and this name minimizes potential > > confusion between p?d_{large,huge}(). > > Agreed. But these fallback also need to first check non-availability of large > pages. We're deliberately not making the p?d_large() helpers generic, so this shouldn't fall back on those. It's up to the architecture to implement these correctly, and the architecture-specific implementations will be added in subsequent patches. Thanks, Mark.
On 07/29/2019 05:08 PM, Steven Price wrote: > On 28/07/2019 12:44, Anshuman Khandual wrote: >> >> >> On 07/23/2019 03:11 PM, Mark Rutland wrote: >>> On Mon, Jul 22, 2019 at 04:41:59PM +0100, Steven Price wrote: >>>> 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 all p?d_leaf() macros, provide >>>> generic do nothing default that are suitable where there cannot be leaf >>>> pages that that level. >>>> >>>> Signed-off-by: Steven Price <steven.price@arm.com> >>> >>> Not a big deal, but it would probably make sense for this to be patch 1 >>> in the series, given it defines the semantic of p?d_leaf(), and they're >>> not used until we provide all the architectural implemetnations anyway. >> >> Agreed. >> >>> >>> It might also be worth pointing out the reasons for this naming, e.g. >>> p?d_large() aren't currently generic, and this name minimizes potential >>> confusion between p?d_{large,huge}(). >> >> Agreed. But these fallback also need to first check non-availability of large >> pages. I am not sure whether CONFIG_HUGETLB_PAGE config being clear indicates >> that conclusively or not. Being a page table leaf entry has a broader meaning >> than a large page but that is really not the case today. All leaf entries here >> are large page entries from MMU perspective. This dependency can definitely be >> removed when there are other types of leaf entries but for now IMHO it feels >> bit problematic not to directly associate leaf entries with large pages in >> config restriction while doing exactly the same. > > The intention here is that the page walkers are able to walk any type of > page table entry which the kernel may use. CONFIG_HUGETLB_PAGE only > controls whether "huge TLB pages" are used by user space processes. It's > quite possible that option to not be selected but the linear mapping to > have been mapped using "large pages" (i.e. leaf entries further up the > tree than normal). I understand that kernel page table might use large pages where as user space never enabled HugeTLB. The point to make here was CONFIG_HUGETLB approximately indicates the presence of large pages though the absence of same does not conclusively indicate that large pages are really absent on the MMU. Perhaps it will requires something new like MMU_[LARGE|HUGE]_PAGES. > > One of the goals was to avoid tying the new functions to a configuration > option but instead match the hardware architecture. Of course this isn't > possible in the most general case (e.g. an architecture may have > multiple hardware page table formats). But to the extent that other > functions like p?d_none() work the desire is that p?d_leaf() should also > work. It is fair enough to assume that a platform can decide wisely and provide accurate definition for p?d_leaf() functions. Anyways its okay not to make this more complex by tying with a new config option which does not exist.
On 07/29/2019 06:20 PM, Mark Rutland wrote: > On Sun, Jul 28, 2019 at 05:14:31PM +0530, Anshuman Khandual wrote: >> On 07/23/2019 03:11 PM, Mark Rutland wrote: >>> It might also be worth pointing out the reasons for this naming, e.g. >>> p?d_large() aren't currently generic, and this name minimizes potential >>> confusion between p?d_{large,huge}(). >> >> Agreed. But these fallback also need to first check non-availability of large >> pages. > > We're deliberately not making the p?d_large() helpers generic, so this > shouldn't fall back on those. I meant non-availability of large page support in the MMU HW not just the presence of p?d_large() helpers.
On 01/08/2019 07:09, Anshuman Khandual wrote: > > > On 07/29/2019 05:08 PM, Steven Price wrote: >> On 28/07/2019 12:44, Anshuman Khandual wrote: >>> >>> >>> On 07/23/2019 03:11 PM, Mark Rutland wrote: >>>> On Mon, Jul 22, 2019 at 04:41:59PM +0100, Steven Price wrote: >>>>> 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 all p?d_leaf() macros, provide >>>>> generic do nothing default that are suitable where there cannot be leaf >>>>> pages that that level. >>>>> >>>>> Signed-off-by: Steven Price <steven.price@arm.com> >>>> >>>> Not a big deal, but it would probably make sense for this to be patch 1 >>>> in the series, given it defines the semantic of p?d_leaf(), and they're >>>> not used until we provide all the architectural implemetnations anyway. >>> >>> Agreed. >>> >>>> >>>> It might also be worth pointing out the reasons for this naming, e.g. >>>> p?d_large() aren't currently generic, and this name minimizes potential >>>> confusion between p?d_{large,huge}(). >>> >>> Agreed. But these fallback also need to first check non-availability of large >>> pages. I am not sure whether CONFIG_HUGETLB_PAGE config being clear indicates >>> that conclusively or not. Being a page table leaf entry has a broader meaning >>> than a large page but that is really not the case today. All leaf entries here >>> are large page entries from MMU perspective. This dependency can definitely be >>> removed when there are other types of leaf entries but for now IMHO it feels >>> bit problematic not to directly associate leaf entries with large pages in >>> config restriction while doing exactly the same. >> >> The intention here is that the page walkers are able to walk any type of >> page table entry which the kernel may use. CONFIG_HUGETLB_PAGE only >> controls whether "huge TLB pages" are used by user space processes. It's >> quite possible that option to not be selected but the linear mapping to >> have been mapped using "large pages" (i.e. leaf entries further up the >> tree than normal). > > I understand that kernel page table might use large pages where as user space > never enabled HugeTLB. The point to make here was CONFIG_HUGETLB approximately > indicates the presence of large pages though the absence of same does not > conclusively indicate that large pages are really absent on the MMU. Perhaps it > will requires something new like MMU_[LARGE|HUGE]_PAGES. CONFIG_HUGETLB doesn't necessarily mean leaf entries can appear anywhere other than PTE. Some architectures always have a full tree of page tables, but can program their TLBs with larger entries - I think all the architectures I've come across have software page table walking, but in theory the arm64 contiguous hint bit could be considered similar. Steve
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h index 75d9d68a6de7..46275896ca66 100644 --- a/include/asm-generic/pgtable.h +++ b/include/asm-generic/pgtable.h @@ -1188,4 +1188,23 @@ static inline bool arch_has_pfn_modify_check(void) #define mm_pmd_folded(mm) __is_defined(__PAGETABLE_PMD_FOLDED) #endif +/* + * p?d_leaf() - 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_leaf +#define pgd_leaf(x) 0 +#endif +#ifndef p4d_leaf +#define p4d_leaf(x) 0 +#endif +#ifndef pud_leaf +#define pud_leaf(x) 0 +#endif +#ifndef pmd_leaf +#define pmd_leaf(x) 0 +#endif + #endif /* _ASM_GENERIC_PGTABLE_H */
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 all p?d_leaf() macros, provide generic do nothing default that are suitable where there cannot be leaf pages that that level. Signed-off-by: Steven Price <steven.price@arm.com> --- include/asm-generic/pgtable.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)