Re: [v3,2/3] powerpc: get hugetlbpage handling more generic

[Scott Wood]

On Fri, 2016-11-25 at 09:14 +0100, Christophe LEROY wrote:
> 
> Le 24/11/2016 Ã 06:23, Scott Wood a Ãcrit :
> > 
> > On Wed, Sep 21, 2016 at 10:11:54AM +0200, Christophe Leroy wrote:
> > > 
> > > Today there are two implementations of hugetlbpages which are managed
> > > by exclusive #ifdefs:
> > > * FSL_BOOKE: several directory entries points to the same single
> > > hugepage
> > > * BOOK3S: one upper level directory entry points to a table of hugepages
> > > 
> > > In preparation of implementation of hugepage support on the 8xx, we
> > > need a mix of the two above solutions, because the 8xx needs both cases
> > > depending on the size of pages:
> > > * In 4k page size mode, each PGD entry covers a 4M bytes area. It means
> > > that 2 PGD entries will be necessary to cover an 8M hugepage while a
> > > single PGD entry will cover 8x 512k hugepages.
> > > * In 16 page size mode, each PGD entry covers a 64M bytes area. It means
> > > that 8x 8M hugepages will be covered by one PGD entry and 64x 512k
> > > hugepages will be covers by one PGD entry.
> > > 
> > > This patch:
> > > * removes #ifdefs in favor of if/else based on the range sizes
> > > * merges the two huge_pte_alloc() functions as they are pretty similar
> > > * merges the two hugetlbpage_init() functions as they are pretty similar
> > > 
> > > Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
> > > Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> > With this patch on e6500, running the hugetlb testsuite results in the
> > system hanging in a storm of OOM killer invocations (I'll try to debug
> > more deeply later).ÂÂThis patch also changes the default hugepage size on
> > FSL book3e from 4M to 16M.
> > 
> Regarding the default hugepage size, it is a result of the merge of theÂ
> two hugetlbpage_init().
> Should I add an ifdef to get 4M on FSL book3e by default ?
> What's the reason for selecting different hugepage sizes depending onÂ
> the CPU ?Â

I'm not sure what the original reason was, but a change in defaults could
disturb existing users.

>ÂI thought default size was selected based on what was existing.

...by code that doesn't expect 4M to ever exist. ÂBoth 4M and 16M (and a bunch
of other sizes) are available on FSL book3e.

What is the reason for preferring 16M over 1M, but preferring 1M over 2M?
ÂSeems arbitrary.

If we want to preserve the exsiting behavior without an ifdef, we could put a
check for 4M before the other sizes, with a comment explaining why we're
making the selection look even more arbitrary. ÂOr we could try to figure out
what size actually makes a better default.

> What testsuite do you run exactly ?

The one that comes with libhugetlbfs (not a particularly recent version, but
not sure exactly how old).

-Scott