Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
Le 12/02/2018 à 00:34, Nicholas Piggin a écrit : On Sun, 11 Feb 2018 21:04:42 +0530 "Aneesh Kumar K.V"wrote: On 02/11/2018 07:29 PM, Nicholas Piggin wrote: On Sat, 10 Feb 2018 13:54:27 +0100 (CET) Christophe Leroy wrote: 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. This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 On this last point, I think it would be better to put these with the existing slice bitmap functions in slice.c and just have a few #ifdefs for SLICE_NUM_HIGH == 0. We went back and forth with that. IMHO, we should avoid as much #ifdef as possible across platforms. It helps to understand the platform restrictions better as we have less and less access to these platforms. The above change indicates that nohash 32 wants to use the slice code and they have different restrictions. With that we now know that book3s64 and nohash 32 are the two different configs using slice code. I don't think it's the right place to put it. It's not platform dependent so much as it just depends on whether or not you have 0 high slices as a workaround for bitmap API not accepting 0 length. Another platform that uses the slice code would just have to copy and paste either the nop or the bitmap implementation depending if it has high slices. So I don't think it's the right abstraction. And it implies a bitmap operation but it very specifically only works for struct slice_mask.high_slices bitmap, which is not clear. Better to just work with struct slice_mask. Some ifdefs inside .c code for small helper functions like this IMO isn't really a big deal -- it's not worse than having it in headers. You just want to avoid ifdef mess when looking at non-trivial logic. static inline void slice_or_mask(struct slice_mask *dst, struct slice_mask *src) { dst->low_slices |= src->low_slices; #if SLICE_NUM_HIGH > 0 bitmap_or(result, dst->high_slices, src->high_slices, SLICE_NUM_HIGH); #endif } I think that's pretty fine. If you have a singular hatred for ifdef in .c, then if() works just as well. To be honest, I tend to agree with you. Therefore, after a few words with Michael, v5 gets rid of those obscure wrappers to come back to the initial (v1) approach which was to use 'if (SLICE_NUM_HIGH)'. Behind the fact that it in my mind looks cleared in the code than using slice_bitmap_xxx() wrappers, it also has the advantage of significantly reducing the size of the patch, which is a must to be able to get it applied on stable. Christophe
Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
Le 12/02/2018 à 00:34, Nicholas Piggin a écrit : On Sun, 11 Feb 2018 21:04:42 +0530 "Aneesh Kumar K.V" wrote: On 02/11/2018 07:29 PM, Nicholas Piggin wrote: On Sat, 10 Feb 2018 13:54:27 +0100 (CET) Christophe Leroy wrote: 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. This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 On this last point, I think it would be better to put these with the existing slice bitmap functions in slice.c and just have a few #ifdefs for SLICE_NUM_HIGH == 0. We went back and forth with that. IMHO, we should avoid as much #ifdef as possible across platforms. It helps to understand the platform restrictions better as we have less and less access to these platforms. The above change indicates that nohash 32 wants to use the slice code and they have different restrictions. With that we now know that book3s64 and nohash 32 are the two different configs using slice code. I don't think it's the right place to put it. It's not platform dependent so much as it just depends on whether or not you have 0 high slices as a workaround for bitmap API not accepting 0 length. Another platform that uses the slice code would just have to copy and paste either the nop or the bitmap implementation depending if it has high slices. So I don't think it's the right abstraction. And it implies a bitmap operation but it very specifically only works for struct slice_mask.high_slices bitmap, which is not clear. Better to just work with struct slice_mask. Some ifdefs inside .c code for small helper functions like this IMO isn't really a big deal -- it's not worse than having it in headers. You just want to avoid ifdef mess when looking at non-trivial logic. static inline void slice_or_mask(struct slice_mask *dst, struct slice_mask *src) { dst->low_slices |= src->low_slices; #if SLICE_NUM_HIGH > 0 bitmap_or(result, dst->high_slices, src->high_slices, SLICE_NUM_HIGH); #endif } I think that's pretty fine. If you have a singular hatred for ifdef in .c, then if() works just as well. To be honest, I tend to agree with you. Therefore, after a few words with Michael, v5 gets rid of those obscure wrappers to come back to the initial (v1) approach which was to use 'if (SLICE_NUM_HIGH)'. Behind the fact that it in my mind looks cleared in the code than using slice_bitmap_xxx() wrappers, it also has the advantage of significantly reducing the size of the patch, which is a must to be able to get it applied on stable. Christophe
Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
Christophe Leroywrites: > 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. > > This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 > Reviewed-by: Aneesh Kumar K.V > Signed-off-by: Christophe Leroy > --- > v2: First patch of v1 serie split in two parts ; added slice_bitmap_xxx() > macros. > v3: Moving slice related stuff in slice.h and slice_32/64.h > slice_bitmap_xxx() are now static inline functions and platform dependent > SLICE_LOW_TOP declared ull on PPC32 with correct casts allows to keep it > 0x1 > v4: Moved slice_32.h and slice_64.h to respective subarch dirs > Moved somes #ifdefs from asm/slice.h to respective subarch slice.h > SLICE_LOW_ details distributed in repective subarch slices allthough > they are identical for the moment > > arch/powerpc/include/asm/book3s/64/slice.h | 79 > ++ > arch/powerpc/include/asm/nohash/32/slice.h | 65 > arch/powerpc/include/asm/nohash/64/slice.h | 12 + > arch/powerpc/include/asm/page.h| 1 + > arch/powerpc/include/asm/page_64.h | 59 -- > arch/powerpc/include/asm/slice.h | 42 > arch/powerpc/mm/slice.c| 38 -- > 7 files changed, 221 insertions(+), 75 deletions(-) > create mode 100644 arch/powerpc/include/asm/book3s/64/slice.h > create mode 100644 arch/powerpc/include/asm/nohash/32/slice.h > create mode 100644 arch/powerpc/include/asm/nohash/64/slice.h > create mode 100644 arch/powerpc/include/asm/slice.h > > diff --git a/arch/powerpc/include/asm/book3s/64/slice.h > b/arch/powerpc/include/asm/book3s/64/slice.h > new file mode 100644 > index ..f9a2c8bd7a77 > --- /dev/null > +++ b/arch/powerpc/include/asm/book3s/64/slice.h > @@ -0,0 +1,79 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_POWERPC_BOOK3S_64_SLICE_H > +#define _ASM_POWERPC_BOOK3S_64_SLICE_H > + > +#ifdef CONFIG_PPC_MM_SLICES > + > +#define SLICE_LOW_SHIFT 28 > +#define SLICE_LOW_TOP(0x1ul) > +#define SLICE_NUM_LOW(SLICE_LOW_TOP >> SLICE_LOW_SHIFT) > +#define GET_LOW_SLICE_INDEX(addr)((addr) >> SLICE_LOW_SHIFT) > + > +#define SLICE_HIGH_SHIFT 40 > +#define SLICE_NUM_HIGH (H_PGTABLE_RANGE >> SLICE_HIGH_SHIFT) > +#define GET_HIGH_SLICE_INDEX(addr) ((addr) >> SLICE_HIGH_SHIFT) > + > +#ifndef __ASSEMBLY__ > + > +#include > + > +static inline void slice_bitmap_zero(unsigned long *dst, unsigned int nbits) > +{ > + bitmap_zero(dst, nbits); > +} > + > +static inline int slice_bitmap_and(unsigned long *dst, > +const unsigned long *src1, > +const unsigned long *src2, > +unsigned int nbits) > +{ > + return bitmap_and(dst, src1, src2, nbits); > +} > + > +static inline void slice_bitmap_or(unsigned long *dst, > +const unsigned long *src1, > +const unsigned long *src2, > +unsigned int nbits) > +{ > + bitmap_or(dst, src1, src2, nbits); > +} > + > +static inline int slice_bitmap_andnot(unsigned long *dst, > + const unsigned long *src1, > + const unsigned long *src2, > + unsigned int nbits) > +{ > + return bitmap_andnot(dst, src1, src2, nbits); > +} > + > +static inline int slice_bitmap_equal(const unsigned long *src1, > + const unsigned long *src2, > + unsigned int nbits) > +{ > + return bitmap_equal(src1, src2, nbits); > +} > + > +static inline int slice_bitmap_empty(const unsigned long *src, unsigned > nbits) > +{ > + return bitmap_empty(src, nbits); > +} > + > +static inline void slice_bitmap_set(unsigned long *map, unsigned int start, > + unsigned int nbits) > +{ > + bitmap_set(map, start, nbits); > +} > +#endif /* __ASSEMBLY__ */ > + > +#else /* CONFIG_PPC_MM_SLICES */ > + > +#define get_slice_psize(mm, addr)((mm)->context.user_psize) > +#define slice_set_user_psize(mm, psize) \ > +do { \ > + (mm)->context.user_psize = (psize); \ > + (mm)->context.sllp = SLB_VSID_USER | mmu_psize_defs[(psize)].sllp; \ > +} while
Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
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. > > This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 > Reviewed-by: Aneesh Kumar K.V > Signed-off-by: Christophe Leroy > --- > v2: First patch of v1 serie split in two parts ; added slice_bitmap_xxx() > macros. > v3: Moving slice related stuff in slice.h and slice_32/64.h > slice_bitmap_xxx() are now static inline functions and platform dependent > SLICE_LOW_TOP declared ull on PPC32 with correct casts allows to keep it > 0x1 > v4: Moved slice_32.h and slice_64.h to respective subarch dirs > Moved somes #ifdefs from asm/slice.h to respective subarch slice.h > SLICE_LOW_ details distributed in repective subarch slices allthough > they are identical for the moment > > arch/powerpc/include/asm/book3s/64/slice.h | 79 > ++ > arch/powerpc/include/asm/nohash/32/slice.h | 65 > arch/powerpc/include/asm/nohash/64/slice.h | 12 + > arch/powerpc/include/asm/page.h| 1 + > arch/powerpc/include/asm/page_64.h | 59 -- > arch/powerpc/include/asm/slice.h | 42 > arch/powerpc/mm/slice.c| 38 -- > 7 files changed, 221 insertions(+), 75 deletions(-) > create mode 100644 arch/powerpc/include/asm/book3s/64/slice.h > create mode 100644 arch/powerpc/include/asm/nohash/32/slice.h > create mode 100644 arch/powerpc/include/asm/nohash/64/slice.h > create mode 100644 arch/powerpc/include/asm/slice.h > > diff --git a/arch/powerpc/include/asm/book3s/64/slice.h > b/arch/powerpc/include/asm/book3s/64/slice.h > new file mode 100644 > index ..f9a2c8bd7a77 > --- /dev/null > +++ b/arch/powerpc/include/asm/book3s/64/slice.h > @@ -0,0 +1,79 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_POWERPC_BOOK3S_64_SLICE_H > +#define _ASM_POWERPC_BOOK3S_64_SLICE_H > + > +#ifdef CONFIG_PPC_MM_SLICES > + > +#define SLICE_LOW_SHIFT 28 > +#define SLICE_LOW_TOP(0x1ul) > +#define SLICE_NUM_LOW(SLICE_LOW_TOP >> SLICE_LOW_SHIFT) > +#define GET_LOW_SLICE_INDEX(addr)((addr) >> SLICE_LOW_SHIFT) > + > +#define SLICE_HIGH_SHIFT 40 > +#define SLICE_NUM_HIGH (H_PGTABLE_RANGE >> SLICE_HIGH_SHIFT) > +#define GET_HIGH_SLICE_INDEX(addr) ((addr) >> SLICE_HIGH_SHIFT) > + > +#ifndef __ASSEMBLY__ > + > +#include > + > +static inline void slice_bitmap_zero(unsigned long *dst, unsigned int nbits) > +{ > + bitmap_zero(dst, nbits); > +} > + > +static inline int slice_bitmap_and(unsigned long *dst, > +const unsigned long *src1, > +const unsigned long *src2, > +unsigned int nbits) > +{ > + return bitmap_and(dst, src1, src2, nbits); > +} > + > +static inline void slice_bitmap_or(unsigned long *dst, > +const unsigned long *src1, > +const unsigned long *src2, > +unsigned int nbits) > +{ > + bitmap_or(dst, src1, src2, nbits); > +} > + > +static inline int slice_bitmap_andnot(unsigned long *dst, > + const unsigned long *src1, > + const unsigned long *src2, > + unsigned int nbits) > +{ > + return bitmap_andnot(dst, src1, src2, nbits); > +} > + > +static inline int slice_bitmap_equal(const unsigned long *src1, > + const unsigned long *src2, > + unsigned int nbits) > +{ > + return bitmap_equal(src1, src2, nbits); > +} > + > +static inline int slice_bitmap_empty(const unsigned long *src, unsigned > nbits) > +{ > + return bitmap_empty(src, nbits); > +} > + > +static inline void slice_bitmap_set(unsigned long *map, unsigned int start, > + unsigned int nbits) > +{ > + bitmap_set(map, start, nbits); > +} > +#endif /* __ASSEMBLY__ */ > + > +#else /* CONFIG_PPC_MM_SLICES */ > + > +#define get_slice_psize(mm, addr)((mm)->context.user_psize) > +#define slice_set_user_psize(mm, psize) \ > +do { \ > + (mm)->context.user_psize = (psize); \ > + (mm)->context.sllp = SLB_VSID_USER | mmu_psize_defs[(psize)].sllp; \ > +} while (0) > + > +#endif /* CONFIG_PPC_MM_SLICES */ > + > +#endif /*
Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
On Sun, 11 Feb 2018 21:04:42 +0530 "Aneesh Kumar K.V"wrote: > On 02/11/2018 07:29 PM, Nicholas Piggin wrote: > > On Sat, 10 Feb 2018 13:54:27 +0100 (CET) > > Christophe Leroy wrote: > > > >> 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. > >> > >> This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 > > > > On this last point, I think it would be better to put these with the > > existing slice bitmap functions in slice.c and just have a few #ifdefs > > for SLICE_NUM_HIGH == 0. > > > > We went back and forth with that. IMHO, we should avoid as much #ifdef > as possible across platforms. It helps to understand the platform > restrictions better as we have less and less access to these platforms. > The above change indicates that nohash 32 wants to use the slice code > and they have different restrictions. With that we now know that > book3s64 and nohash 32 are the two different configs using slice code. I don't think it's the right place to put it. It's not platform dependent so much as it just depends on whether or not you have 0 high slices as a workaround for bitmap API not accepting 0 length. Another platform that uses the slice code would just have to copy and paste either the nop or the bitmap implementation depending if it has high slices. So I don't think it's the right abstraction. And it implies a bitmap operation but it very specifically only works for struct slice_mask.high_slices bitmap, which is not clear. Better to just work with struct slice_mask. Some ifdefs inside .c code for small helper functions like this IMO isn't really a big deal -- it's not worse than having it in headers. You just want to avoid ifdef mess when looking at non-trivial logic. static inline void slice_or_mask(struct slice_mask *dst, struct slice_mask *src) { dst->low_slices |= src->low_slices; #if SLICE_NUM_HIGH > 0 bitmap_or(result, dst->high_slices, src->high_slices, SLICE_NUM_HIGH); #endif } I think that's pretty fine. If you have a singular hatred for ifdef in .c, then if() works just as well. Thanks, Nick
Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
On Sun, 11 Feb 2018 21:04:42 +0530 "Aneesh Kumar K.V" wrote: > On 02/11/2018 07:29 PM, Nicholas Piggin wrote: > > On Sat, 10 Feb 2018 13:54:27 +0100 (CET) > > Christophe Leroy wrote: > > > >> 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. > >> > >> This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 > > > > On this last point, I think it would be better to put these with the > > existing slice bitmap functions in slice.c and just have a few #ifdefs > > for SLICE_NUM_HIGH == 0. > > > > We went back and forth with that. IMHO, we should avoid as much #ifdef > as possible across platforms. It helps to understand the platform > restrictions better as we have less and less access to these platforms. > The above change indicates that nohash 32 wants to use the slice code > and they have different restrictions. With that we now know that > book3s64 and nohash 32 are the two different configs using slice code. I don't think it's the right place to put it. It's not platform dependent so much as it just depends on whether or not you have 0 high slices as a workaround for bitmap API not accepting 0 length. Another platform that uses the slice code would just have to copy and paste either the nop or the bitmap implementation depending if it has high slices. So I don't think it's the right abstraction. And it implies a bitmap operation but it very specifically only works for struct slice_mask.high_slices bitmap, which is not clear. Better to just work with struct slice_mask. Some ifdefs inside .c code for small helper functions like this IMO isn't really a big deal -- it's not worse than having it in headers. You just want to avoid ifdef mess when looking at non-trivial logic. static inline void slice_or_mask(struct slice_mask *dst, struct slice_mask *src) { dst->low_slices |= src->low_slices; #if SLICE_NUM_HIGH > 0 bitmap_or(result, dst->high_slices, src->high_slices, SLICE_NUM_HIGH); #endif } I think that's pretty fine. If you have a singular hatred for ifdef in .c, then if() works just as well. Thanks, Nick
Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
On 02/11/2018 07:29 PM, Nicholas Piggin wrote: On Sat, 10 Feb 2018 13:54:27 +0100 (CET) Christophe Leroywrote: 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. This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 On this last point, I think it would be better to put these with the existing slice bitmap functions in slice.c and just have a few #ifdefs for SLICE_NUM_HIGH == 0. We went back and forth with that. IMHO, we should avoid as much #ifdef as possible across platforms. It helps to understand the platform restrictions better as we have less and less access to these platforms. The above change indicates that nohash 32 wants to use the slice code and they have different restrictions. With that we now know that book3s64 and nohash 32 are the two different configs using slice code. -aneesh
Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
On 02/11/2018 07:29 PM, Nicholas Piggin wrote: On Sat, 10 Feb 2018 13:54:27 +0100 (CET) Christophe Leroy wrote: 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. This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 On this last point, I think it would be better to put these with the existing slice bitmap functions in slice.c and just have a few #ifdefs for SLICE_NUM_HIGH == 0. We went back and forth with that. IMHO, we should avoid as much #ifdef as possible across platforms. It helps to understand the platform restrictions better as we have less and less access to these platforms. The above change indicates that nohash 32 wants to use the slice code and they have different restrictions. With that we now know that book3s64 and nohash 32 are the two different configs using slice code. -aneesh
Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
On Sat, 10 Feb 2018 13:54:27 +0100 (CET) Christophe Leroywrote: > 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. > > This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 On this last point, I think it would be better to put these with the existing slice bitmap functions in slice.c and just have a few #ifdefs for SLICE_NUM_HIGH == 0. Thanks, Nick
Re: [PATCH v4 2/5] powerpc/mm/slice: Enhance for supporting PPC32
On Sat, 10 Feb 2018 13:54:27 +0100 (CET) Christophe Leroy wrote: > 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. > > This patch moves "slices" functions prototypes from page64.h to slice.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() functions which will void on PPC32 On this last point, I think it would be better to put these with the existing slice bitmap functions in slice.c and just have a few #ifdefs for SLICE_NUM_HIGH == 0. Thanks, Nick