Re: [PATCH v2 1/5] powerpc/mm: Enhance 'slice' for supporting PPC32

[Christophe LEROY]

Le 19/01/2018 Ã 10:13, Aneesh Kumar K.V a ÃcritÂ:
> On 01/19/2018 02:37 PM, Christophe LEROY wrote:
> > Le 19/01/2018 Ã 10:02, Aneesh Kumar K.V a ÃcritÂ:
> > > On 01/19/2018 02:14 PM, Christophe LEROY wrote:
> > > > Le 19/01/2018 Ã 09:24, Aneesh Kumar K.V a ÃcritÂ:
> > > > > Christophe Leroy <christophe.leroy@xxxxxx> writes:
> > > > >
> > > > > > In preparation for the following patch which will fix an issue on
> > > > > > the 8xx by re-using the 'slices', this patch enhances the
> > > > > > 'slices' implementation to support 32 bits CPUs.
> > > > > >
> > > > > > On PPC32, the address space is limited to 4Gbytes, hence only the low
> > > > > > slices will be used. As of today, the code uses
> > > > > > SLICE_LOW_TOP (0x100000000ul) and compares it with addr to determine
> > > > > > if addr refers to low or high space.
> > > > > > On PPC32, such a (addr < SLICE_LOW_TOP) test is always false because
> > > > > > 0x100000000ul degrades to 0. Therefore, the patch modifies
> > > > > > SLICE_LOW_TOP to (0xfffffffful) and modifies the tests to
> > > > > > (addr <= SLICE_LOW_TOP) which will then always be true on PPC32
> > > > > > as addr has type 'unsigned long' while not modifying the PPC64
> > > > > > behaviour.
> > > > > >
> > > > > > This patch moves "slices" functions prototypes from page64.h to 
> > > > > > page.h
> > > > > >
> > > > > > The high slices use bitmaps. As bitmap functions are not prepared to
> > > > > > handling bitmaps of size 0, the bitmap_xxx() calls are wrapped into
> > > > > > slice_bitmap_xxx() macros which will take care of the 0 nbits case.
> > > > > >
> > > > > > Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
> > > > > > ---
> > > > > > Â v2: First patch of v1 serie split in two parts ; added 
> > > > > > slice_bitmap_xxx() macros.
> > > > > >
> > > > > > Â arch/powerpc/include/asm/page.hÂÂÂÂÂ | 14 +++++++++
> > > > > > Â arch/powerpc/include/asm/page_32.hÂÂ | 19 ++++++++++++
> > > > > > Â arch/powerpc/include/asm/page_64.hÂÂ | 21 ++-----------
> > > > > > Â arch/powerpc/mm/hash_utils_64.cÂÂÂÂÂ |Â 2 +-
> > > > > > Â arch/powerpc/mm/mmu_context_nohash.c |Â 7 +++++
> > > > > > Â arch/powerpc/mm/slice.cÂÂÂÂÂÂÂÂÂÂÂÂÂ | 60 
> > > > > > ++++++++++++++++++++++++------------
> > > > > > Â 6 files changed, 83 insertions(+), 40 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/powerpc/include/asm/page.h 
> > > > > > b/arch/powerpc/include/asm/page.h
> > > > > > index 8da5d4c1cab2..d0384f9db9eb 100644
> > > > > > --- a/arch/powerpc/include/asm/page.h
> > > > > > +++ b/arch/powerpc/include/asm/page.h
> > > > > > @@ -342,6 +342,20 @@ typedef struct page *pgtable_t;
> > > > > > Â #endif
> > > > > > Â #endif
> > > > > > +#ifdef CONFIG_PPC_MM_SLICES
> > > > > > +struct mm_struct;
> > > > > > +
> > > > > > +unsigned long slice_get_unmapped_area(unsigned long addr, 
> > > > > > unsigned long len,
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long flags, unsigned int psize,
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int topdown);
> > > > > > +
> > > > > > +unsigned int get_slice_psize(struct mm_struct *mm, unsigned long 
> > > > > > addr);
> > > > > > +
> > > > > > +void slice_set_user_psize(struct mm_struct *mm, unsigned int psize);
> > > > > > +void slice_set_range_psize(struct mm_struct *mm, unsigned long 
> > > > > > start,
> > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long len, unsigned int psize);
> > > > > > +#endif
> > > > > > +
> > > > >
> > > > > Should we do a slice.h ? the way we have other files? and then do
> > > >
> > > > Yes we could add a slice.h instead of using page.h for that, good idea.
> > > >
> > > > > arch/powerpc/include/asm/book3s/64/slice.h that will carry
> > > > > #define slice_bitmap_zero(dst, nbits) \
> > > > > ÂÂÂÂdo { if (nbits) bitmap_zero(dst, nbits); } while (0)
> > > > > #define slice_bitmap_set(dst, pos, nbits) \
> > > > > do { if (nbits) bitmap_set(dst, pos, nbits); } while (0)
> > > > > #define slice_bitmap_copy(dst, src, nbits) \
> > > > > do { if (nbits) bitmap_copy(dst, src, nbits); } while (0)
> > > > > #define slice_bitmap_and(dst, src1, src2, nbits) \
> > > > > ÂÂÂÂ({ (nbits) ? bitmap_and(dst, src1, src2, nbits) : 0; })
> > > > > #define slice_bitmap_or(dst, src1, src2, nbits) \
> > > > > ÂÂÂÂdo { if (nbits) bitmap_or(dst, src1, src2, nbits); } while (0)
> > > > > #define slice_bitmap_andnot(dst, src1, src2, nbits) \
> > > > > ÂÂÂÂ({ (nbits) ? bitmap_andnot(dst, src1, src2, nbits) : 0; })
> > > > > #define slice_bitmap_equal(src1, src2, nbits) \
> > > > > ÂÂÂÂ({ (nbits) ? bitmap_equal(src1, src2, nbits) : 1; })
> > > > > #define slice_bitmap_empty(src, nbits) \
> > > > > ÂÂÂÂ({ (nbits) ? bitmap_empty(src, nbits) : 1; })
> > > > >
> > > > > This without that if(nbits) check and a proper static inline so 
> > > > > that we
> > > > > can do type checking.
> > > >
> > > > Is it really worth duplicating that just for eliminating the 'if 
> > > > (nbits)' in one case ?
> > > >
> > > > Only in book3s/64 we will be able to eliminate that, for nohash/32 
> > > > we need to keep the test due to the difference between low and high 
> > > > slices.
> > >
> > > the other advantage is we move the SLICE_LOW_SHIFT to the right 
> > > location. IMHO mm subystem is really complex with these really 
> > > overloaded headers. If we can keep it seperate we should with 
> > > minimal code duplication?
> >
> > For the constants I fully agree with your proposal and I will do it. I 
> > was only questionning the benefit of moving the slice_bitmap_xxxx() 
> > stuff, taking into account that the 'if (nbits)' test is already 
> > eliminated by the compiler.
>
> That is compiler dependent as you are finding with the other patch where 
> if (0) didn't get compiled out

I don't think so. When I had the missing prototype, the compilation goes 
ok, including the final link. Which means at the end the code is not 
included since radix_enabled() evaluates to 0.

Many many parts of the kernel are based on this assumption.

Christophe

> -aneesh