Re: [RFC PATCH v2 1/5] mm: make HPAGE_PxD_{SHIFT,MASK,SIZE} always available
On Fri, Jul 10, 2020 at 05:57:46PM +0100, Matthew Wilcox wrote:
> On Fri, Jul 10, 2020 at 12:40:37PM -0400, Andrea Arcangeli wrote:
> > Hello Hugh and Mike,
> >
> > On Mon, Jul 06, 2020 at 10:07:34PM -0700, Hugh Dickins wrote:
> > > Adding Andrea to Cc, he's the one who structured it that way,
> > > and should be consulted.
> > >
> > > I'm ambivalent myself. Many's the time I've been irritated by the
> > > BUILD_BUG() in HPAGE_etc, and it's responsible for very many #ifdef
> > > CONFIG_TRANSPARENT_HUGEPAGEs or IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)s
> > > that you find uglily scattered around the source.
> > >
> > > But that's the point of it: it's warning when you write code peculiar
> > > to THP, that is going to bloat the build of kernels without any THP.
> > >
> > > So although I've often been tempted to do as you suggest, I've always
> > > ended up respecting Andrea's intention, and worked around it instead
> > > (sometimes with #ifdef or IS_ENABLED(), sometimes with
> > > PMD_{SHIFT,MASK_SIZE}, sometimes with a local definition).
> >
> > The only other reasons that comes to mind in addition of optimizing
> > the bloat away at build time is to make it easier to identify the THP
> > code and to make it explicit that hugetlbfs shouldn't us it or it
> > could be wrong on some arches.
> >
> > However for this case the BUILD_BUG() looks right and this doesn't
> > look like a false positive.
> >
> > This patchset has nothing to do THP, so it'd be more correct to use
> > MAX_ORDER whenever the fragmentation is about the buddy (doesn't look
> > the case here) or PUD_SIZE/ORDER/PMD_SIZE/ORDER if the objective is
> > not to unnecessarily split extra and unrelated hugepud/hugepmds in the
> > direct mapping (as in this case).
> >
> > The real issue exposed by the BUILD_BUG is the lack of PMD_ORDER
> > definition and fs/dax.c already run into and it solved it locally in the
> > dax.c file:
> >
> > /* The order of a PMD entry */
> > #define PMD_ORDER (PMD_SHIFT - PAGE_SHIFT)
> >
> > The fact it's not just this patch but also dax.c that run into the
> > same issue, makes me think PMD_ORDER should be defined and then you
> > can use PMD_* and PUD_* for this non-THP purpose.
>
> We'll run into some namespace issues.
>
> arch/arm/kernel/head.S:#define PMD_ORDER3
> arch/arm/kernel/head.S:#define PMD_ORDER2
> arch/mips/include/asm/pgtable-32.h:#define PMD_ORDER
> ai_attempt_to_allocate_pmd
> arch/mips/include/asm/pgtable-64.h:#define PMD_ORDER0
> arch/parisc/include/asm/pgtable.h:#define PMD_ORDER 1 /* Number of pages
> per pmd */
This can be easily solved with, e.g.
#define PMD_PAGE_ORDER (PMD_SHIFT - PAGE_SHIFT)
or by renaming the current defines to PMD_ALLOC_ORDER.
> > Then the question if to remove the BUILD_BUG becomes orthogonal to
> > this patchset, but I don't see much value in retaining HPAGE_PMD/PUD_*
> > unless the BUILD_BUG is retained too, because this patchset already
> > hints that without the BUILD_BUG() the HPAGE_PMD_* definitions would
> > likely spill into non THP paths and they would lose also the only
> > value left (the ability to localize the THP code paths). So I wouldn't
> > be against removing the BUILD_BUG if it's causing maintenance
> > overhead, but then I would drop HPAGE_PMD_* too along with it or it
> > may just cause confusion.
>
> btw, using the hpage_ prefix already caused one problem in the hugetlb
> code:
>
> https://lore.kernel.org/linux-mm/20200629185003.97202-1-mike.krav...@oracle.com/
>
> I'd suggest we rename these to THP_PMD_* and THP_PUD_* to make it clear
> they're only for the THP case.
I agree that THP_PMD_* and THP_PUD_* would be less confusing if we are
to differentiate THP and non-THP usage of 2nd and 3rd level leaf pages.
--
Sincerely yours,
Mike.
Re: [RFC PATCH v2 1/5] mm: make HPAGE_PxD_{SHIFT,MASK,SIZE} always available
On Fri, Jul 10, 2020 at 05:57:46PM +0100, Matthew Wilcox wrote: > btw, using the hpage_ prefix already caused one problem in the hugetlb > code: > > https://lore.kernel.org/linux-mm/20200629185003.97202-1-mike.krav...@oracle.com/ > > I'd suggest we rename these to THP_PMD_* and THP_PUD_* to make it clear > they're only for the THP case. The confusion seem to have happened only about hpage_nr_pages not about HPAGE_PMD_*. It's just the hpage_ prefix alone that is commonly used by hugetlbfs only and so it's not surprising it caused confusion. So I certainly agree hpage_nr_pages would better be renamed to something more THP specific (either hpage_pmd_nr_pages or trans_huge_nr_pages or as you wish), but HPAGE_PMD_ don't look too confusing about the fact it's only for the THP case since the non-THP case won't necessarily care about PMDs. Thanks, Andrea
Re: [RFC PATCH v2 1/5] mm: make HPAGE_PxD_{SHIFT,MASK,SIZE} always available
On Fri, Jul 10, 2020 at 12:40:37PM -0400, Andrea Arcangeli wrote:
> Hello Hugh and Mike,
>
> On Mon, Jul 06, 2020 at 10:07:34PM -0700, Hugh Dickins wrote:
> > Adding Andrea to Cc, he's the one who structured it that way,
> > and should be consulted.
> >
> > I'm ambivalent myself. Many's the time I've been irritated by the
> > BUILD_BUG() in HPAGE_etc, and it's responsible for very many #ifdef
> > CONFIG_TRANSPARENT_HUGEPAGEs or IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)s
> > that you find uglily scattered around the source.
> >
> > But that's the point of it: it's warning when you write code peculiar
> > to THP, that is going to bloat the build of kernels without any THP.
> >
> > So although I've often been tempted to do as you suggest, I've always
> > ended up respecting Andrea's intention, and worked around it instead
> > (sometimes with #ifdef or IS_ENABLED(), sometimes with
> > PMD_{SHIFT,MASK_SIZE}, sometimes with a local definition).
>
> The only other reasons that comes to mind in addition of optimizing
> the bloat away at build time is to make it easier to identify the THP
> code and to make it explicit that hugetlbfs shouldn't us it or it
> could be wrong on some arches.
>
> However for this case the BUILD_BUG() looks right and this doesn't
> look like a false positive.
>
> This patchset has nothing to do THP, so it'd be more correct to use
> MAX_ORDER whenever the fragmentation is about the buddy (doesn't look
> the case here) or PUD_SIZE/ORDER/PMD_SIZE/ORDER if the objective is
> not to unnecessarily split extra and unrelated hugepud/hugepmds in the
> direct mapping (as in this case).
>
> The real issue exposed by the BUILD_BUG is the lack of PMD_ORDER
> definition and fs/dax.c already run into and it solved it locally in the
> dax.c file:
>
> /* The order of a PMD entry */
> #define PMD_ORDER (PMD_SHIFT - PAGE_SHIFT)
>
> The fact it's not just this patch but also dax.c that run into the
> same issue, makes me think PMD_ORDER should be defined and then you
> can use PMD_* and PUD_* for this non-THP purpose.
We'll run into some namespace issues.
arch/arm/kernel/head.S:#define PMD_ORDER3
arch/arm/kernel/head.S:#define PMD_ORDER2
arch/mips/include/asm/pgtable-32.h:#define PMD_ORDER
ai_attempt_to_allocate_pmd
arch/mips/include/asm/pgtable-64.h:#define PMD_ORDER0
arch/parisc/include/asm/pgtable.h:#define PMD_ORDER 1 /* Number of pages
per pmd */
> Then the question if to remove the BUILD_BUG becomes orthogonal to
> this patchset, but I don't see much value in retaining HPAGE_PMD/PUD_*
> unless the BUILD_BUG is retained too, because this patchset already
> hints that without the BUILD_BUG() the HPAGE_PMD_* definitions would
> likely spill into non THP paths and they would lose also the only
> value left (the ability to localize the THP code paths). So I wouldn't
> be against removing the BUILD_BUG if it's causing maintenance
> overhead, but then I would drop HPAGE_PMD_* too along with it or it
> may just cause confusion.
btw, using the hpage_ prefix already caused one problem in the hugetlb
code:
https://lore.kernel.org/linux-mm/20200629185003.97202-1-mike.krav...@oracle.com/
I'd suggest we rename these to THP_PMD_* and THP_PUD_* to make it clear
they're only for the THP case.
Re: [RFC PATCH v2 1/5] mm: make HPAGE_PxD_{SHIFT,MASK,SIZE} always available
Hello Hugh and Mike,
On Mon, Jul 06, 2020 at 10:07:34PM -0700, Hugh Dickins wrote:
> Adding Andrea to Cc, he's the one who structured it that way,
> and should be consulted.
>
> I'm ambivalent myself. Many's the time I've been irritated by the
> BUILD_BUG() in HPAGE_etc, and it's responsible for very many #ifdef
> CONFIG_TRANSPARENT_HUGEPAGEs or IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)s
> that you find uglily scattered around the source.
>
> But that's the point of it: it's warning when you write code peculiar
> to THP, that is going to bloat the build of kernels without any THP.
>
> So although I've often been tempted to do as you suggest, I've always
> ended up respecting Andrea's intention, and worked around it instead
> (sometimes with #ifdef or IS_ENABLED(), sometimes with
> PMD_{SHIFT,MASK_SIZE}, sometimes with a local definition).
The only other reasons that comes to mind in addition of optimizing
the bloat away at build time is to make it easier to identify the THP
code and to make it explicit that hugetlbfs shouldn't us it or it
could be wrong on some arches.
However for this case the BUILD_BUG() looks right and this doesn't
look like a false positive.
This patchset has nothing to do THP, so it'd be more correct to use
MAX_ORDER whenever the fragmentation is about the buddy (doesn't look
the case here) or PUD_SIZE/ORDER/PMD_SIZE/ORDER if the objective is
not to unnecessarily split extra and unrelated hugepud/hugepmds in the
direct mapping (as in this case).
The real issue exposed by the BUILD_BUG is the lack of PMD_ORDER
definition and fs/dax.c already run into and it solved it locally in the
dax.c file:
/* The order of a PMD entry */
#define PMD_ORDER (PMD_SHIFT - PAGE_SHIFT)
The fact it's not just this patch but also dax.c that run into the
same issue, makes me think PMD_ORDER should be defined and then you
can use PMD_* and PUD_* for this non-THP purpose.
Then the question if to remove the BUILD_BUG becomes orthogonal to
this patchset, but I don't see much value in retaining HPAGE_PMD/PUD_*
unless the BUILD_BUG is retained too, because this patchset already
hints that without the BUILD_BUG() the HPAGE_PMD_* definitions would
likely spill into non THP paths and they would lose also the only
value left (the ability to localize the THP code paths). So I wouldn't
be against removing the BUILD_BUG if it's causing maintenance
overhead, but then I would drop HPAGE_PMD_* too along with it or it
may just cause confusion.
Thanks,
Andrea
Re: [RFC PATCH v2 1/5] mm: make HPAGE_PxD_{SHIFT,MASK,SIZE} always available
Hi Hugh,
On Mon, Jul 06, 2020 at 10:07:34PM -0700, Hugh Dickins wrote:
> On Mon, 6 Jul 2020, Mike Rapoport wrote:
> > From: Mike Rapoport
> >
> > The definitions of shift, mask and size for the second and the third level
> > of the leaf pages are available only when CONFIG_TRANSPARENT_HUGEPAGE is
> > set. Otherwise they evaluate to BUILD_BUG().
> >
> > There is no explanation neither in the code nor in the changelog why the
> > usage of, e.g. HPAGE_PMD_SIZE should be only allowed with THP and forbidden
> > otherwise while the definitions of HPAGE_PMD_SIZE and HPAGE_PUD_SIZE
> > express the sizes better than ambiguous HPAGE_SIZE.
> >
> > Make HPAGE_PxD_{SHIFT,MASK,SIZE} definitions available unconditionally.
>
> Adding Andrea to Cc, he's the one who structured it that way,
> and should be consulted.
>
> I'm ambivalent myself. Many's the time I've been irritated by the
> BUILD_BUG() in HPAGE_etc, and it's responsible for very many #ifdef
> CONFIG_TRANSPARENT_HUGEPAGEs or IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)s
> that you find uglily scattered around the source.
>
> But that's the point of it: it's warning when you write code peculiar
> to THP, that is going to bloat the build of kernels without any THP.
>
> So although I've often been tempted to do as you suggest, I've always
> ended up respecting Andrea's intention, and worked around it instead
> (sometimes with #ifdef or IS_ENABLED(), sometimes with
> PMD_{SHIFT,MASK_SIZE}, sometimes with a local definition).
I could do with a local definition as well, but I think HPAGE_PxD_SHIFT
is better and more descriptive than ambiguous HPAGE_SHIFT and I was
thinking about wider change to use "THP" defines rather than "hugetlb"
defines wherever possible.
In the end, HPAGE_PMD_SIZE does not have to be associated with THP and
limited to it, it just says what is the size of a leaf page at PMD
level.
> Hugh
>
> >
> > Signed-off-by: Mike Rapoport
> > ---
> > include/linux/huge_mm.h | 10 ++
> > 1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 71f20776b06c..1f4b44a76e31 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -115,7 +115,6 @@ extern struct kobj_attribute shmem_enabled_attr;
> > #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT)
> > #define HPAGE_PMD_NR (1< >
> > -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > #define HPAGE_PMD_SHIFT PMD_SHIFT
> > #define HPAGE_PMD_SIZE ((1UL) << HPAGE_PMD_SHIFT)
> > #define HPAGE_PMD_MASK (~(HPAGE_PMD_SIZE - 1))
> > @@ -124,6 +123,8 @@ extern struct kobj_attribute shmem_enabled_attr;
> > #define HPAGE_PUD_SIZE ((1UL) << HPAGE_PUD_SHIFT)
> > #define HPAGE_PUD_MASK (~(HPAGE_PUD_SIZE - 1))
> >
> > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > +
> > extern unsigned long transparent_hugepage_flags;
> >
> > /*
> > @@ -316,13 +317,6 @@ static inline struct list_head
> > *page_deferred_list(struct page *page)
> > }
> >
> > #else /* CONFIG_TRANSPARENT_HUGEPAGE */
> > -#define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
> > -#define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
> > -#define HPAGE_PMD_SIZE ({ BUILD_BUG(); 0; })
> > -
> > -#define HPAGE_PUD_SHIFT ({ BUILD_BUG(); 0; })
> > -#define HPAGE_PUD_MASK ({ BUILD_BUG(); 0; })
> > -#define HPAGE_PUD_SIZE ({ BUILD_BUG(); 0; })
> >
> > static inline int hpage_nr_pages(struct page *page)
> > {
> > --
> > 2.26.2
--
Sincerely yours,
Mike.
Re: [RFC PATCH v2 1/5] mm: make HPAGE_PxD_{SHIFT,MASK,SIZE} always available
On Mon, 6 Jul 2020, Mike Rapoport wrote:
> From: Mike Rapoport
>
> The definitions of shift, mask and size for the second and the third level
> of the leaf pages are available only when CONFIG_TRANSPARENT_HUGEPAGE is
> set. Otherwise they evaluate to BUILD_BUG().
>
> There is no explanation neither in the code nor in the changelog why the
> usage of, e.g. HPAGE_PMD_SIZE should be only allowed with THP and forbidden
> otherwise while the definitions of HPAGE_PMD_SIZE and HPAGE_PUD_SIZE
> express the sizes better than ambiguous HPAGE_SIZE.
>
> Make HPAGE_PxD_{SHIFT,MASK,SIZE} definitions available unconditionally.
Adding Andrea to Cc, he's the one who structured it that way,
and should be consulted.
I'm ambivalent myself. Many's the time I've been irritated by the
BUILD_BUG() in HPAGE_etc, and it's responsible for very many #ifdef
CONFIG_TRANSPARENT_HUGEPAGEs or IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)s
that you find uglily scattered around the source.
But that's the point of it: it's warning when you write code peculiar
to THP, that is going to bloat the build of kernels without any THP.
So although I've often been tempted to do as you suggest, I've always
ended up respecting Andrea's intention, and worked around it instead
(sometimes with #ifdef or IS_ENABLED(), sometimes with
PMD_{SHIFT,MASK_SIZE}, sometimes with a local definition).
Hugh
>
> Signed-off-by: Mike Rapoport
> ---
> include/linux/huge_mm.h | 10 ++
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 71f20776b06c..1f4b44a76e31 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -115,7 +115,6 @@ extern struct kobj_attribute shmem_enabled_attr;
> #define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT)
> #define HPAGE_PMD_NR (1<
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> #define HPAGE_PMD_SHIFT PMD_SHIFT
> #define HPAGE_PMD_SIZE ((1UL) << HPAGE_PMD_SHIFT)
> #define HPAGE_PMD_MASK (~(HPAGE_PMD_SIZE - 1))
> @@ -124,6 +123,8 @@ extern struct kobj_attribute shmem_enabled_attr;
> #define HPAGE_PUD_SIZE ((1UL) << HPAGE_PUD_SHIFT)
> #define HPAGE_PUD_MASK (~(HPAGE_PUD_SIZE - 1))
>
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +
> extern unsigned long transparent_hugepage_flags;
>
> /*
> @@ -316,13 +317,6 @@ static inline struct list_head
> *page_deferred_list(struct page *page)
> }
>
> #else /* CONFIG_TRANSPARENT_HUGEPAGE */
> -#define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
> -#define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
> -#define HPAGE_PMD_SIZE ({ BUILD_BUG(); 0; })
> -
> -#define HPAGE_PUD_SHIFT ({ BUILD_BUG(); 0; })
> -#define HPAGE_PUD_MASK ({ BUILD_BUG(); 0; })
> -#define HPAGE_PUD_SIZE ({ BUILD_BUG(); 0; })
>
> static inline int hpage_nr_pages(struct page *page)
> {
> --
> 2.26.2
[RFC PATCH v2 1/5] mm: make HPAGE_PxD_{SHIFT,MASK,SIZE} always available
From: Mike Rapoport
The definitions of shift, mask and size for the second and the third level
of the leaf pages are available only when CONFIG_TRANSPARENT_HUGEPAGE is
set. Otherwise they evaluate to BUILD_BUG().
There is no explanation neither in the code nor in the changelog why the
usage of, e.g. HPAGE_PMD_SIZE should be only allowed with THP and forbidden
otherwise while the definitions of HPAGE_PMD_SIZE and HPAGE_PUD_SIZE
express the sizes better than ambiguous HPAGE_SIZE.
Make HPAGE_PxD_{SHIFT,MASK,SIZE} definitions available unconditionally.
Signed-off-by: Mike Rapoport
---
include/linux/huge_mm.h | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 71f20776b06c..1f4b44a76e31 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -115,7 +115,6 @@ extern struct kobj_attribute shmem_enabled_attr;
#define HPAGE_PMD_ORDER (HPAGE_PMD_SHIFT-PAGE_SHIFT)
#define HPAGE_PMD_NR (1<
