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

From: christophe leroy
Date: Sat Jan 20 2018 - 03:23:13 EST

Hi Segher,

Le 19/01/2018 Ã 10:45, Christophe LEROY a ÃcritÂ:


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.



Segher, what is your opinion on the above ? Can we consider that a ' if (nbits)' will always be compiled out when nbits is a #define constant, or should we duplicate the macros as suggested in order to avoid unneccessary 'if' test on platforms where 'nbits' is always not null by definition ?

Patch is at https://patchwork.ozlabs.org/patch/862117/

Christophe

---
L'absence de virus dans ce courrier Ãlectronique a Ãtà vÃrifiÃe par le logiciel antivirus Avast.
https://www.avast.com/antivirus