[PATCH] powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[PATCH] powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

MAX_PHYSMEM_BITS only needs to be defined if CONFIG_SPARSEMEM is
enabled, and that was the case before commit 4ffe713b7587
("powerpc/mm: Increase the max addressable memory to 2PB").

On 32-bit systems, where CONFIG_SPARSEMEM is not enabled, we now
define it as 46.  That is larger than the real number of physical
address bits, and breaks calculations in zsmalloc:

    mm/zsmalloc.c:130:49: warning: right shift count is negative [-Wshift-count-negative]
      MAX(32, (ZS_MAX_PAGES_PER_ZSPAGE << PAGE_SHIFT >> OBJ_INDEX_BITS))
                                                     ^~
    ...
    mm/zsmalloc.c:253:21: error: variably modified 'size_class' at file scope
      struct size_class *size_class[ZS_SIZE_CLASSES];
                         ^~~~~~~~~~

Fixes: 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB")
Cc: stable@vger.kernel.org
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 arch/powerpc/include/asm/mmu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index 25607604a7a5..2bc8c3f04f8a 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -341,7 +341,7 @@ static inline u16 get_mm_addr_key(struct mm_struct *mm, unsigned long address)
 #if defined(CONFIG_SPARSEMEM_VMEMMAP) && defined(CONFIG_SPARSEMEM_EXTREME) &&	\
 	defined (CONFIG_PPC_64K_PAGES)
 #define MAX_PHYSMEM_BITS        51
-#else
+#elif defined(CONFIG_SPARSEMEM)
 #define MAX_PHYSMEM_BITS        46
 #endif
 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Michael Ellerman]

On Sun, 2019-03-17 at 01:17:56 UTC, Ben Hutchings wrote:
> MAX_PHYSMEM_BITS only needs to be defined if CONFIG_SPARSEMEM is
> enabled, and that was the case before commit 4ffe713b7587
> ("powerpc/mm: Increase the max addressable memory to 2PB").
> 
> On 32-bit systems, where CONFIG_SPARSEMEM is not enabled, we now
> define it as 46.  That is larger than the real number of physical
> address bits, and breaks calculations in zsmalloc:
> 
>     mm/zsmalloc.c:130:49: warning: right shift count is negative [-Wshift-count-negative]
>       MAX(32, (ZS_MAX_PAGES_PER_ZSPAGE << PAGE_SHIFT >> OBJ_INDEX_BITS))
>                                                      ^~
>     ...
>     mm/zsmalloc.c:253:21: error: variably modified 'size_class' at file scope
>       struct size_class *size_class[ZS_SIZE_CLASSES];
>                          ^~~~~~~~~~
> 
> Fixes: 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/8bc086899816214fbc6047c9c7e15fca

cheers

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Andreas Schwab]

On Mär 22 2019, Michael Ellerman <patch-notifications@ellerman.id.au> wrote:

> On Sun, 2019-03-17 at 01:17:56 UTC, Ben Hutchings wrote:
>> MAX_PHYSMEM_BITS only needs to be defined if CONFIG_SPARSEMEM is
>> enabled, and that was the case before commit 4ffe713b7587
>> ("powerpc/mm: Increase the max addressable memory to 2PB").
>> 
>> On 32-bit systems, where CONFIG_SPARSEMEM is not enabled, we now
>> define it as 46.  That is larger than the real number of physical
>> address bits, and breaks calculations in zsmalloc:
>> 
>>     mm/zsmalloc.c:130:49: warning: right shift count is negative [-Wshift-count-negative]
>>       MAX(32, (ZS_MAX_PAGES_PER_ZSPAGE << PAGE_SHIFT >> OBJ_INDEX_BITS))
>>                                                      ^~
>>     ...
>>     mm/zsmalloc.c:253:21: error: variably modified 'size_class' at file scope
>>       struct size_class *size_class[ZS_SIZE_CLASSES];
>>                          ^~~~~~~~~~
>> 
>> Fixes: 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
>
> Applied to powerpc fixes, thanks.
>
> https://git.kernel.org/powerpc/c/8bc086899816214fbc6047c9c7e15fca

In file included from ./arch/powerpc/include/asm/book3s/64/mmu.h:39:0,
                 from ./arch/powerpc/include/asm/mmu.h:360,
                 from ./arch/powerpc/include/asm/lppaca.h:36,
                 from ./arch/powerpc/include/asm/paca.h:21,
                 from ./arch/powerpc/include/asm/current.h:16,
                 from ./include/linux/thread_info.h:21,
                 from ./include/asm-generic/preempt.h:5,
                 from ./arch/powerpc/include/generated/asm/preempt.h:1,
                 from ./include/linux/preempt.h:78,
                 from ./include/linux/spinlock.h:51,
                 from ./include/linux/mmzone.h:8,
                 from ./include/linux/gfp.h:6,
                 from ./include/linux/slab.h:15,
                 from ./include/linux/crypto.h:24,
                 from ./include/crypto/algapi.h:15,
                 from ./include/crypto/internal/hash.h:16,
                 from arch/powerpc/crypto/md5-glue.c:15:
./arch/powerpc/include/asm/book3s/64/mmu-hash.h:584:6: error: "MAX_PHYSMEM_BITS" is not defined [-Werror=undef]
 #if (MAX_PHYSMEM_BITS > MAX_EA_BITS_PER_CONTEXT)
      ^

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3363 bytes --]

On Sun, 2019-03-24 at 23:18 +0100, Andreas Schwab wrote:
> On Mär 22 2019, Michael Ellerman <patch-notifications@ellerman.id.au> wrote:
> 
> > On Sun, 2019-03-17 at 01:17:56 UTC, Ben Hutchings wrote:
> > > MAX_PHYSMEM_BITS only needs to be defined if CONFIG_SPARSEMEM is
> > > enabled, and that was the case before commit 4ffe713b7587
> > > ("powerpc/mm: Increase the max addressable memory to 2PB").
> > > 
> > > On 32-bit systems, where CONFIG_SPARSEMEM is not enabled, we now
> > > define it as 46.  That is larger than the real number of physical
> > > address bits, and breaks calculations in zsmalloc:
> > > 
> > >     mm/zsmalloc.c:130:49: warning: right shift count is negative [-Wshift-count-negative]
> > >       MAX(32, (ZS_MAX_PAGES_PER_ZSPAGE << PAGE_SHIFT >> OBJ_INDEX_BITS))
> > >                                                      ^~
> > >     ...
> > >     mm/zsmalloc.c:253:21: error: variably modified 'size_class' at file scope
> > >       struct size_class *size_class[ZS_SIZE_CLASSES];
> > >                          ^~~~~~~~~~
> > > 
> > > Fixes: 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > 
> > Applied to powerpc fixes, thanks.
> > 
> > https://git.kernel.org/powerpc/c/8bc086899816214fbc6047c9c7e15fca
> 
> In file included from ./arch/powerpc/include/asm/book3s/64/mmu.h:39:0,
>                  from ./arch/powerpc/include/asm/mmu.h:360,
>                  from ./arch/powerpc/include/asm/lppaca.h:36,
>                  from ./arch/powerpc/include/asm/paca.h:21,
>                  from ./arch/powerpc/include/asm/current.h:16,
>                  from ./include/linux/thread_info.h:21,
>                  from ./include/asm-generic/preempt.h:5,
>                  from ./arch/powerpc/include/generated/asm/preempt.h:1,
>                  from ./include/linux/preempt.h:78,
>                  from ./include/linux/spinlock.h:51,
>                  from ./include/linux/mmzone.h:8,
>                  from ./include/linux/gfp.h:6,
>                  from ./include/linux/slab.h:15,
>                  from ./include/linux/crypto.h:24,
>                  from ./include/crypto/algapi.h:15,
>                  from ./include/crypto/internal/hash.h:16,
>                  from arch/powerpc/crypto/md5-glue.c:15:
> ./arch/powerpc/include/asm/book3s/64/mmu-hash.h:584:6: error: "MAX_PHYSMEM_BITS" is not defined [-Werror=undef]
>  #if (MAX_PHYSMEM_BITS > MAX_EA_BITS_PER_CONTEXT)
>       ^

Presumably you have CONFIG_PPC_BOOK3S_64 enabled and CONFIG_SPARSEMEM
disabled?  Was this configuration actually usable?

I think this would fix the build failure:

--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -341,7 +341,7 @@ static inline u16 get_mm_addr_key(struct mm_struct
*mm, unsigned long address)
 #if defined(CONFIG_SPARSEMEM_VMEMMAP) &&
defined(CONFIG_SPARSEMEM_EXTREME) &&  \
        defined (CONFIG_PPC_64K_PAGES)
 #define MAX_PHYSMEM_BITS        51
-#elif defined(CONFIG_SPARSEMEM)
+#elif defined(CONFIG_PPC64)
 #define MAX_PHYSMEM_BITS        46
 #endif
 
--- END ---

but whether the result will actually *work*, I don't know.

Ben.

-- 
Ben Hutchings
Larkinson's Law: All laws are basically false.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Andreas Schwab]

On Mär 24 2019, Ben Hutchings <ben@decadent.org.uk> wrote:

> Presumably you have CONFIG_PPC_BOOK3S_64 enabled and CONFIG_SPARSEMEM
> disabled?  Was this configuration actually usable?

Why not?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Mon, 2019-03-25 at 01:03 +0100, Andreas Schwab wrote:
> On Mär 24 2019, Ben Hutchings <ben@decadent.org.uk> wrote:
> 
> > Presumably you have CONFIG_PPC_BOOK3S_64 enabled and
> > CONFIG_SPARSEMEM
> > disabled?  Was this configuration actually usable?
> 
> Why not?

I assume that CONFIG_SPARSEMEM is the default for a good reason.
What I don't know is how strong that reason is (I am not a Power expert
at all).  Looking a bit further, it seems to be related to CONFIG_NUMA
in that you can enable CONFIG_FLATMEM if and only if that's disabled. 
So I suppose the configuration you used works for non-NUMA systems.

Anyway, does the patch I sent work for you?

Ben.

-- 
Ben Hutchings
I'm not a reverse psychological virus.
Please don't copy me into your signature.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Michael Ellerman]

Ben Hutchings <ben@decadent.org.uk> writes:
> On Mon, 2019-03-25 at 01:03 +0100, Andreas Schwab wrote:
>> On Mär 24 2019, Ben Hutchings <ben@decadent.org.uk> wrote:
>> 
>> > Presumably you have CONFIG_PPC_BOOK3S_64 enabled and
>> > CONFIG_SPARSEMEM
>> > disabled?  Was this configuration actually usable?
>> 
>> Why not?
>
> I assume that CONFIG_SPARSEMEM is the default for a good reason.
> What I don't know is how strong that reason is (I am not a Power expert
> at all).  Looking a bit further, it seems to be related to CONFIG_NUMA
> in that you can enable CONFIG_FLATMEM if and only if that's disabled. 
> So I suppose the configuration you used works for non-NUMA systems.

Aneesh pointed out this fix would break FLATMEM after I'd merged it, but
it didn't break any of our defconfigs so I wondered if anyone would
notice.

I checked today and a G5 will boot with FLATMEM, which I assume is what
Andreas is using.

I guess we should fix this build break for now.

Even some G5's have discontiguous memory, so FLATMEM is not clearly a
good choice even for all G5's, and actually a fresh g5_defconfig uses
SPARSEMEM.

So I'm inclined to just switch to always using SPARSEMEM on 64-bit
Book3S, because that's what's well tested and we hardly need more code
paths to test. Unless anyone has a strong objection, I haven't actually
benchmarked FLATMEM vs SPARSEMEM on a G5.

cheers

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Christophe Leroy]

Le 25/03/2019 à 12:35, Michael Ellerman a écrit :
> Ben Hutchings <ben@decadent.org.uk> writes:
>> On Mon, 2019-03-25 at 01:03 +0100, Andreas Schwab wrote:
>>> On Mär 24 2019, Ben Hutchings <ben@decadent.org.uk> wrote:
>>>
>>>> Presumably you have CONFIG_PPC_BOOK3S_64 enabled and
>>>> CONFIG_SPARSEMEM
>>>> disabled?  Was this configuration actually usable?
>>>
>>> Why not?
>>
>> I assume that CONFIG_SPARSEMEM is the default for a good reason.
>> What I don't know is how strong that reason is (I am not a Power expert
>> at all).  Looking a bit further, it seems to be related to CONFIG_NUMA
>> in that you can enable CONFIG_FLATMEM if and only if that's disabled.
>> So I suppose the configuration you used works for non-NUMA systems.
> 
> Aneesh pointed out this fix would break FLATMEM after I'd merged it, but
> it didn't break any of our defconfigs so I wondered if anyone would
> notice.
> 
> I checked today and a G5 will boot with FLATMEM, which I assume is what
> Andreas is using.
> 
> I guess we should fix this build break for now.
> 
> Even some G5's have discontiguous memory, so FLATMEM is not clearly a
> good choice even for all G5's, and actually a fresh g5_defconfig uses
> SPARSEMEM.
> 
> So I'm inclined to just switch to always using SPARSEMEM on 64-bit
> Book3S, because that's what's well tested and we hardly need more code
> paths to test. Unless anyone has a strong objection, I haven't actually
> benchmarked FLATMEM vs SPARSEMEM on a G5.
> 

Christian Zigotzky reported a build failure with this patch.

Christophe

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Andreas Schwab]

On Mär 25 2019, Michael Ellerman <mpe@ellerman.id.au> wrote:

> So I'm inclined to just switch to always using SPARSEMEM on 64-bit
> Book3S, because that's what's well tested and we hardly need more code
> paths to test. Unless anyone has a strong objection, I haven't actually
> benchmarked FLATMEM vs SPARSEMEM on a G5.

Configuring with SPARSEMEM saves about 32Mb of memory.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Aneesh Kumar K.V]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Ben Hutchings <ben@decadent.org.uk> writes:
>> On Mon, 2019-03-25 at 01:03 +0100, Andreas Schwab wrote:
>>> On Mär 24 2019, Ben Hutchings <ben@decadent.org.uk> wrote:
>>> 
>>> > Presumably you have CONFIG_PPC_BOOK3S_64 enabled and
>>> > CONFIG_SPARSEMEM
>>> > disabled?  Was this configuration actually usable?
>>> 
>>> Why not?
>>
>> I assume that CONFIG_SPARSEMEM is the default for a good reason.
>> What I don't know is how strong that reason is (I am not a Power expert
>> at all).  Looking a bit further, it seems to be related to CONFIG_NUMA
>> in that you can enable CONFIG_FLATMEM if and only if that's disabled. 
>> So I suppose the configuration you used works for non-NUMA systems.
>
> Aneesh pointed out this fix would break FLATMEM after I'd merged it, but
> it didn't break any of our defconfigs so I wondered if anyone would
> notice.
>
> I checked today and a G5 will boot with FLATMEM, which I assume is what
> Andreas is using.
>
> I guess we should fix this build break for now.
>
> Even some G5's have discontiguous memory, so FLATMEM is not clearly a
> good choice even for all G5's, and actually a fresh g5_defconfig uses
> SPARSEMEM.
>
> So I'm inclined to just switch to always using SPARSEMEM on 64-bit
> Book3S, because that's what's well tested and we hardly need more code
> paths to test. Unless anyone has a strong objection, I haven't actually
> benchmarked FLATMEM vs SPARSEMEM on a G5.
>

How about

From 207fb0036065d8db44853e63bb858c4fd9952106 Mon Sep 17 00:00:00 2001
From: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Date: Mon, 1 Apr 2019 17:51:17 +0530
Subject: [PATCH] powerpc/mm: Fix build error 

The current value of MAX_PHYSMEM_BITS cannot work with 32 bit configs.
We used to have MAX_PHYSMEM_BITS not defined without SPARSEMEM and 32
bit configs never expected a value to be set for MAX_PHYSMEM_BITS.

Dependent code such as zsmalloc derived the right values based on other
fields. Instead of finding a value that works with different configs,
use new values only for book3s_64. For 64 bit booke, use the definition
of MAX_PHYSMEM_BITS as per commit a7df61a0e2b6 ("[PATCH] ppc64: Increase sparsemem defaults")
That change was done in 2005 and hopefully will work with book3e 64.

Fixes: 4ffe713b7587 ("powerpc/mm: Increase the max addressable memory to 2PB")
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 arch/powerpc/include/asm/book3s/64/mmu.h | 15 +++++++++++++++
 arch/powerpc/include/asm/mmu.h           | 15 ---------------
 arch/powerpc/include/asm/nohash/64/mmu.h |  2 ++
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/include/asm/book3s/64/mmu.h
index 1ceee000c18d..a809bdd77322 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu.h
@@ -35,6 +35,21 @@ typedef pte_t *pgtable_t;
 
 #endif /* __ASSEMBLY__ */
 
+/*
+ * If we store section details in page->flags we can't increase the MAX_PHYSMEM_BITS
+ * if we increase SECTIONS_WIDTH we will not store node details in page->flags and
+ * page_to_nid does a page->section->node lookup
+ * Hence only increase for VMEMMAP. Further depending on SPARSEMEM_EXTREME reduce
+ * memory requirements with large number of sections.
+ * 51 bits is the max physical real address on POWER9
+ */
+#if defined(CONFIG_SPARSEMEM_VMEMMAP) && defined(CONFIG_SPARSEMEM_EXTREME) &&  \
+	defined(CONFIG_PPC_64K_PAGES)
+#define MAX_PHYSMEM_BITS 51
+#else
+#define MAX_PHYSMEM_BITS 46
+#endif
+
 /* 64-bit classic hash table MMU */
 #include <asm/book3s/64/mmu-hash.h>
 
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index 598cdcdd1355..78d53c4396ac 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -341,21 +341,6 @@ static inline bool strict_kernel_rwx_enabled(void)
  */
 #define MMU_PAGE_COUNT	16
 
-/*
- * If we store section details in page->flags we can't increase the MAX_PHYSMEM_BITS
- * if we increase SECTIONS_WIDTH we will not store node details in page->flags and
- * page_to_nid does a page->section->node lookup
- * Hence only increase for VMEMMAP. Further depending on SPARSEMEM_EXTREME reduce
- * memory requirements with large number of sections.
- * 51 bits is the max physical real address on POWER9
- */
-#if defined(CONFIG_SPARSEMEM_VMEMMAP) && defined(CONFIG_SPARSEMEM_EXTREME) &&	\
-	defined (CONFIG_PPC_64K_PAGES)
-#define MAX_PHYSMEM_BITS        51
-#elif defined(CONFIG_SPARSEMEM)
-#define MAX_PHYSMEM_BITS        46
-#endif
-
 #ifdef CONFIG_PPC_BOOK3S_64
 #include <asm/book3s/64/mmu.h>
 #else /* CONFIG_PPC_BOOK3S_64 */
diff --git a/arch/powerpc/include/asm/nohash/64/mmu.h b/arch/powerpc/include/asm/nohash/64/mmu.h
index e6585480dfc4..81cf30c370e5 100644
--- a/arch/powerpc/include/asm/nohash/64/mmu.h
+++ b/arch/powerpc/include/asm/nohash/64/mmu.h
@@ -2,6 +2,8 @@
 #ifndef _ASM_POWERPC_NOHASH_64_MMU_H_
 #define _ASM_POWERPC_NOHASH_64_MMU_H_
 
+#define MAX_PHYSMEM_BITS        44
+
 /* Freescale Book-E software loaded TLB or Book-3e (ISA 2.06+) MMU */
 #include <asm/nohash/mmu-book3e.h>
 
-- 
2.20.1

Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations

[Rui Salvaterra]

Hi, Ben

> Presumably you have CONFIG_PPC_BOOK3S_64 enabled and CONFIG_SPARSEMEM disabled?
> Was this configuration actually usable?

I just noticed these build warnings too. FWIW, I've been using
CONFIG_FLATMEM on my Power Mac G5 for about three years, so I guess
the answer is "yes" (for this specific machine, at least).