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

[Aneesh Kumar K.V]

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.le...@c-s.fr> 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.le...@c-s.fr>
---
  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

-aneesh