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

[christophe leroy <christophe.leroy@c-s.fr>]

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@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.leroy@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
> 
> 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