Re: [PATCH v8 4/4] hugetlb: allow to free gigantic pages regardless of the configuration

[Alexandre Ghiti]

On 03/27/2019 09:55 AM, Aneesh Kumar K.V wrote:
> On 3/27/19 2:14 PM, Alexandre Ghiti wrote:
> > On 03/27/2019 08:01 AM, Aneesh Kumar K.V wrote:
> > > On 3/27/19 12:06 PM, Alexandre Ghiti wrote:
> > > > On systems without CONTIG_ALLOC activated but that support gigantic 
> > > > pages,
> > > > boottime reserved gigantic pages can not be freed at all. This patch
> > > > simply enables the possibility to hand back those pages to memory
> > > > allocator.
> > > >
> > > > Signed-off-by: Alexandre Ghiti <alex@xxxxxxxx>
> > > > Acked-by: David S. Miller <davem@xxxxxxxxxxxxx> [sparc]
> > > >
> > > > diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h 
> > > > b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> > > > index ec2a55a553c7..7013284f0f1b 100644
> > > > --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
> > > > +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> > > > @@ -36,8 +36,8 @@ static inline int hstate_get_psize(struct hstate 
> > > > *hstate)
> > > > ÂÂÂÂÂ }
> > > > Â }
> > > > Â -#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> > > > -static inline bool gigantic_page_supported(void)
> > > > +#define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
> > > > +static inline bool gigantic_page_runtime_supported(void)
> > > > Â {
> > > > ÂÂÂÂÂ /*
> > > > ÂÂÂÂÂÂ * We used gigantic page reservation with hypervisor assist 
> > > > in some case.
> > > > @@ -49,7 +49,6 @@ static inline bool gigantic_page_supported(void)
> > > > Â ÂÂÂÂÂ return true;
> > > > Â }
> > > > -#endif
> > > > Â Â /* hugepd entry valid bit */
> > > > Â #define HUGEPD_VAL_BITSÂÂÂÂÂÂÂ (0x8000000000000000UL)
> > >
> > > Is that correct when CONTIG_ALLOC is not enabled? I guess we want
> > >
> > > gigantic_page_runtime_supported to return false when CONTIG_ALLOC is 
> > > not enabled on all architectures and on POWER when it is enabled we 
> > > want it to be conditional as it is now.
> > >
> > > -aneesh
> >
> > CONFIG_ARCH_HAS_GIGANTIC_PAGE is set by default when an architecture 
> > supports gigantic
> > pages: on its own, it allows to allocate boottime gigantic pages AND 
> > to free them at runtime
> > (this is the goal of this series), but not to allocate runtime 
> > gigantic pages.
> > If CONTIG_ALLOC is set, it allows in addition to allocate runtime 
> > gigantic pages.
> >
> > I re-introduced the runtime checks because we can't know at compile 
> > time if powerpc can
> > or not support gigantic pages.
> >
> > So for all architectures, gigantic_page_runtime_supported only 
> > depends on
> > CONFIG_ARCH_HAS_GIGANTIC_PAGE enabled or not. The possibility to 
> > allocate runtime
> > gigantic pages is dealt with after those runtime checks.
>
> you removed that #ifdef in the patch above. ie we had
> #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> static inline bool gigantic_page_supported(void)
> {
> ÂÂÂÂ/*
> ÂÂÂÂ * We used gigantic page reservation with hypervisor assist in 
> some case.
> ÂÂÂÂ * We cannot use runtime allocation of gigantic pages in those 
> platforms
> ÂÂÂÂ * This is hash translation mode LPARs.
> ÂÂÂÂ */
> ÂÂÂÂif (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
> ÂÂÂÂÂÂÂ return false;
>
> ÂÂÂÂreturn true;
> }
> #endif

Yes, I removed the #ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE because it was 
defined only
if CONTIG_ALLOC was set. But now, CONFIG_ARCH_HAS_GIGANTIC_PAGE is 
inconditionally
set for powerpc so I think we don't need it anymore.
Actually I have doubts now, is this true for all configurations ? I see 
that it is only set for
PPC_RADIX_MMU. I think the problem is here: instead of returning true, 
it should do like
the generic version, ie return IS_ENABLED(CONFIG_ARCH_HAS_GIGANTIC_PAGE).
Do you agree ?

> This is now
> #define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
> static inline bool gigantic_page_runtime_supported(void)
> {
> if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
> ÂÂÂÂÂÂÂ return false;
>
> ÂÂÂÂreturn true;
> }
>
>
> I am wondering whether it should be
>
> #define __HAVE_ARCH_GIGANTIC_PAGE_RUNTIME_SUPPORTED
> static inline bool gigantic_page_runtime_supported(void)
> {
>
> ÂÂ if (!IS_ENABLED(CONFIG_CONTIG_ALLOC))
> ÂÂÂÂÂÂÂ return false;

I don't think this test should happen here, CONFIG_CONTIG_ALLOC only allows
to allocate gigantic pages, doing that check here would prevent powerpc
to free boottime gigantic pages when not a guest. Note that this check
is actually done in set_max_huge_pages.


> if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
> ÂÂÂÂÂÂÂ return false;

Maybe I did not understand this check: I understood that, in the case 
the system
is virtualized, we do not want it to hand back gigantic pages. Does this 
check
test if the system is currently being virtualized ?
If yes, I think the patch is correct: it prevents freeing gigantic pages 
when the system
is virtualized but allows a 'normal' system to free gigantic pages.


> ÂÂÂÂreturn true;
> }
>
> or add that #ifdef back.
>
> > By the way, I forgot to ask you why you think that if an arch cannot 
> > allocate runtime gigantic
> > pages, it should not be able to free boottime gigantic pages ?
>
> on virtualized platforms like powervm which use a paravirtualized page 
> table update mechanism (we dont' have two level table). The ability to 
> map a page huge depends on how hypervisor allocated the guest ram. 
> Hypervisor also allocates the guest specific page table of a specific 
> size depending on how many pages are going to be mapped by what page 
> size.
>
> on POWER we indicate possible guest real address that can be mapped 
> via hugepage (in this case 16G) using a device tree node 
> (ibm,expected#pages) . It is expected that we will map these pages 
> only as 16G pages. Hence we cannot free them back to the buddy where 
> it could get mapped via 64K page size.

Thanks for the explanations.

Alex
> -aneesh