From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from pegase1.c-s.fr (pegase1.c-s.fr [93.17.236.30]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3zNrL918VWzF0jr for ; Sat, 20 Jan 2018 19:23:11 +1100 (AEDT) Subject: Re: [PATCH v2 1/5] powerpc/mm: Enhance 'slice' for supporting PPC32 From: christophe leroy To: "Aneesh Kumar K.V" , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , Scott Wood , Segher Boessenkool Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org References: <49148d07955d3e5f963cedf9adcfcc37c3e03ef4.1516179904.git.christophe.leroy@c-s.fr> <87vafyz265.fsf@linux.vnet.ibm.com> <84dc1df4-db2f-be11-c1f3-5dddd1e44983@c-s.fr> <28c3ba39-ef31-5ff3-7672-3e9d1942be94@c-s.fr> Message-ID: <36e8d873-4021-4266-bf5f-287f396ba9e1@c-s.fr> Date: Sat, 20 Jan 2018 09:22:50 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 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 >>>>>>> --- >>>>>>>   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