sh: remove unneeded constructor.

pgd_cache specifies __GFP_ZERO when allocating.
This constructor is meaningless.

Signed-off-by Yoshinori Sato <[email protected]>

arch/sh/mm/pgtable.c +0 -8

@@ -9,20 +9,11 @@ static struct kmem_cache *pgd_cachep;
 static struct kmem_cache *pmd_cachep;
 #endif
 
-void pgd_ctor(void *x)
-{
-       pgd_t *pgd = x;
-
-       memcpy(pgd + USER_PTRS_PER_PGD,
-              swapper_pg_dir + USER_PTRS_PER_PGD,
-              (PTRS_PER_PGD - USER_PTRS_PER_PGD) * sizeof(pgd_t));
-}

Hi Sato-san,

CC Rob , Willy, linux-arch

Quoted 4 lines · Yoshinori Sato

pgd_cache specifies __GFP_ZERO when allocating.
This constructor is meaningless.

Signed-off-by Yoshinori Sato <[email protected]>

Thanks for your patch!

Quoted 14 lines

arch/sh/mm/pgtable.c +0 -8

@@ -9,20 +9,11 @@ static struct kmem_cache *pgd_cachep;
 static struct kmem_cache *pmd_cachep;
 #endif
 
-void pgd_ctor(void *x)
-{
-       pgd_t *pgd = x;
-
-       memcpy(pgd + USER_PTRS_PER_PGD,
-              swapper_pg_dir + USER_PTRS_PER_PGD,
-              (PTRS_PER_PGD - USER_PTRS_PER_PGD) * sizeof(pgd_t));
-}

arch/sh/mm/pgtable.c static struct kmem_cache *pgd_cachep; · line 19

Quoted 10 lines

arch/sh/mm/pgtable.c +1 -2

-
 void pgtable_cache_init(void)
 {
        pgd_cachep = kmem_cache_create("pgd_cache",
                                       PTRS_PER_PGD * (1<<PTE_MAGNITUDE),
-                                      PAGE_SIZE, SLAB_PANIC, pgd_ctor);
+                                      PAGE_SIZE, SLAB_PANIC, NULL);
 #if PAGETABLE_LEVELS > 2
        pmd_cachep = kmem_cache_create("pmd_cache",
                                       PTRS_PER_PMD * (1<<PTE_MAGNITUDE),

While I can confirm your patch fixes the

    WARNING: CPU: 0 PID: 1 at mm/slub.c:2412
___slab_alloc.constprop.34+0x196/0x288

seen since commit 128227e7fe4087b6 ("slab: __GFP_ZERO is incompatible with a constructor"), I'm not 100% sure this is the correct fix.

swapper_pg_dir[] does have two non-zero entries (768 and 895), which were copied by the constructor. Perhaps SH does have a need for these two entries, and thus for the constructor?

Unfortunately I'm too SH-illiterate to answer this myself.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I have not tested enough to impose mm on my part, so it may be by chance this too.
Restore the constructor and modify it so that __ GFP_ZERO is not specified. Because then I think that it is safer because it is exactly the same as before the fix.

Quoted 5 lines · Yoshinori Sato

I have not tested enough to impose mm on my part, so it may be by chance this too.
Restore the constructor and modify it so that __ GFP_ZERO is not specified. Because then I think that it is safer because it is exactly the same as before the fix.

I wish you had cc'd me on patch v2. I think the answer is actually this (which restoers the pre-2009 behaviour):

arch/sh/mm/pgtable.c +2 -1

index 5c8f9247c3c2..7c63aa359c7d 100644
@@ -2,7 +2,7 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 
-#define PGALLOC_GFP GFP_KERNEL | __GFP_ZERO
+#define PGALLOC_GFP GFP_KERNEL
 
 static struct kmem_cache *pgd_cachep;
 #if PAGETABLE_LEVELS > 2

@@ -13,6 +13,7 @@ void pgd_ctor(void *x)
 {
        pgd_t *pgd = x;
 
+       memset(pgd, 0, USER_PTRS_PER_PGD * sizeof(pgd_t));
        memcpy(pgd + USER_PTRS_PER_PGD,
               swapper_pg_dir + USER_PTRS_PER_PGD,
               (PTRS_PER_PGD - USER_PTRS_PER_PGD) * sizeof(pgd_t));

but I haven't even compiled it.

You do want to readd the __GFP_ZERO flag to the second user of PGALLOC_GFP, don't you?

Quoted 2 lines · Geert Uytterhoeven

You do want to readd the __GFP_ZERO flag to the second user of PGALLOC_GFP, don't you?

I missed that! Probably only relevant for SH-64. But yes ... probably better to make this explicit then:

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · Geert Uytterhoeven

You do want to readd the __GFP_ZERO flag to the second user of PGALLOC_GFP, don't you?

I missed that! Probably only relevant for SH-64. But yes ... probably better to make this explicit then:

As far as I know sh5/sh-64 never shipped, it was just some prototype hardware that didn't go to production?

Quoted 10 lines · Rob Landley

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · Geert Uytterhoeven

You do want to readd the __GFP_ZERO flag to the second user of PGALLOC_GFP, don't you?

I missed that! Probably only relevant for SH-64. But yes ... probably better to make this explicit then:

As far as I know sh5/sh-64 never shipped, it was just some prototype hardware that didn't go to production?

I'm not sure about the details, but GCC has removed support and it's effectively dead. I would be happy to merge patches removing the existing SH-64 stuff in the kernel too. I'm not sure if generality to support LP64 should be left in the arch/sh tree for future or if the eventual 64-bit j-core should just be done as a separate arch tree; I suspect the latter might be cleaner.

Quoted 12 lines · Rich Felker

Quoted 10 lines · Rob Landley

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · Geert Uytterhoeven

You do want to readd the __GFP_ZERO flag to the second user of PGALLOC_GFP, don't you?

I missed that! Probably only relevant for SH-64. But yes ... probably better to make this explicit then:

As far as I know sh5/sh-64 never shipped, it was just some prototype hardware that didn't go to production?

Quoted 6 lines

I'm not sure about the details, but GCC has removed support and it's effectively dead. I would be happy to merge patches removing the existing SH-64 stuff in the kernel too. I'm not sure if generality to support LP64 should be left in the arch/sh tree for future or if the eventual 64-bit j-core should just be done as a separate arch tree; I suspect the latter might be cleaner.

LP64 is mostly just "long fits in a pointer", which is true on 32 bit too. What extra "lp64" support are you referring to?

It seems like the only architecture with 32 and 64 bit variants that _doesn't_ have them together is arm? (x86 does, powerpc does, mips does...)

Quoted 20 lines · Rob Landley

Quoted 12 lines · Rich Felker

Quoted 10 lines · Rob Landley

Quoted 6 lines · Matthew Wilcox

Quoted 2 lines · Geert Uytterhoeven

You do want to readd the __GFP_ZERO flag to the second user of PGALLOC_GFP, don't you?

I missed that! Probably only relevant for SH-64. But yes ... probably better to make this explicit then:

As far as I know sh5/sh-64 never shipped, it was just some prototype hardware that didn't go to production?

Quoted 6 lines

I'm not sure about the details, but GCC has removed support and it's effectively dead. I would be happy to merge patches removing the existing SH-64 stuff in the kernel too. I'm not sure if generality to support LP64 should be left in the arch/sh tree for future or if the eventual 64-bit j-core should just be done as a separate arch tree; I suspect the latter might be cleaner.

Quoted 2 lines

LP64 is mostly just "long fits in a pointer", which is true on 32 bit too. What extra "lp64" support are you referring to?

I'm using the normal definition of LP64 which is "long and pointer are both exactly 64-bits", not "long fits in pointer" or "long and pointer are same size".

Quoted 2 lines

It seems like the only architecture with 32 and 64 bit variants that _doesn't_ have them together is arm? (x86 does, powerpc does, mips does...)

Yes, if there's a lot of arch-specific infrastructure for boot time setup, platform devices, behavior of mmu and cache, etc. that's the same for 32- and 64-bit "variants" of an arch, and especially if the 32-bit variant is mostly a legacy userspace that actually runs on 64-bit hardware that's capable of doing both, then there's a good argument for having them use a shared tree. That last part makes sense for SH -- I think we'd certainly want 32-bit userspace to be able to run on a 64-bit kernel. But it seems like it's possible to split that across archs since ARM did.

Really it's sort of a historical accident/misfactoring that "user-to-kernel interface for ISA X" and "kernel internals for the machine arch using ISA X" are conflated as the same thing. If this had been done in a more clean manner the "compat syscall" stuff wouldn't be such a mess.

Quoted 13 lines · Rich Felker

Quoted 1 line · Rob Landley

On 08/05/2018 11:32 AM, Rich Felker wrote:

Yes, if there's a lot of arch-specific infrastructure for boot time setup, platform devices, behavior of mmu and cache, etc. that's the same for 32- and 64-bit "variants" of an arch, and especially if the 32-bit variant is mostly a legacy userspace that actually runs on 64-bit hardware that's capable of doing both, then there's a good argument for having them use a shared tree. That last part makes sense for SH -- I think we'd certainly want 32-bit userspace to be able to run on a 64-bit kernel. But it seems like it's possible to split that across archs since ARM did.

Quoted 5 lines

Really it's sort of a historical accident/misfactoring that "user-to-kernel interface for ISA X" and "kernel internals for the machine arch using ISA X" are conflated as the same thing. If this had been done in a more clean manner the "compat syscall" stuff wouldn't be such a mess.

Obviously you know more about eventual 64-bit j-core support than me, but if you intend to run 32-bit userspace on 64-bit j-core, having it shared makes sense, especially if you'll maintain both.

On the platform side, there will be a big difference between mostly legacy SH and new DT-based j-core on the 32-bit bit side, and mostly new DT-based j-core on the 64-bit side.

Arnd probably has a better view on this.

On the platform side, there will be a big difference between mostly legacy SH and new DT-based j-core on the 32-bit bit side, and mostly new DT-based j-core on the 64-bit side.

Quoted 2 lines · Geert Uytterhoeven

You do want to readd the __GFP_ZERO flag to the second user of PGALLOC_GFP, don't you?

I missed that! Probably only relevant for SH-64. But yes ... probably better to make this explicit then:

+++ b/arch/sh/mm/pgtable.c

arch/sh/mm/pgtable.c +2 -3

@@ -2,8 +2,6 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 
-#define PGALLOC_GFP GFP_KERNEL | __GFP_ZERO
-
 static struct kmem_cache *pgd_cachep;
 #if PAGETABLE_LEVELS > 2
 static struct kmem_cache *pmd_cachep;

@@ -13,6 +11,7 @@ void pgd_ctor(void *x)
 {
        pgd_t *pgd = x;
 
+       memset(pgd, 0, USER_PTRS_PER_PGD * sizeof(pgd_t));
        memcpy(pgd + USER_PTRS_PER_PGD,
               swapper_pg_dir + USER_PTRS_PER_PGD,
               (PTRS_PER_PGD - USER_PTRS_PER_PGD) * sizeof(pgd_t));

@@ -32,7 +31,7 @@ void pgtable_cache_init(void)
 
 pgd_t *pgd_alloc(struct mm_struct *mm)
 {
-       return kmem_cache_alloc(pgd_cachep, PGALLOC_GFP);
+       return kmem_cache_alloc(pgd_cachep, GFP_KERNEL);
 }
 
 void pgd_free(struct mm_struct *mm, pgd_t *pgd)

On Sat, Aug 04, 2018 at 03:51:49AM -0700, Matthew Wilcox wrote:

I've been asked to include this in this or next pull request, and I think it's right to do so, but I don't have a complete patch from you. Can you resubmit with a commit message and signed-off-by?

Hi Sato-san,

CC Rob , Willy, linux-arch

Quoted 8 lines

arch/sh/mm/pgtable.c +0 -8

-void pgd_ctor(void *x)
-{
-       pgd_t *pgd = x;
-
-       memcpy(pgd + USER_PTRS_PER_PGD,
-              swapper_pg_dir + USER_PTRS_PER_PGD,
-              (PTRS_PER_PGD - USER_PTRS_PER_PGD) * sizeof(pgd_t));
-}

arch/sh/mm/pgtable.c static struct kmem_cache *pgd_cachep; · line 19

Quoted 10 lines

arch/sh/mm/pgtable.c +1 -2

-
 void pgtable_cache_init(void)
 {
        pgd_cachep = kmem_cache_create("pgd_cache",
                                       PTRS_PER_PGD * (1<<PTE_MAGNITUDE),
-                                      PAGE_SIZE, SLAB_PANIC, pgd_ctor);
+                                      PAGE_SIZE, SLAB_PANIC, NULL);
 #if PAGETABLE_LEVELS > 2
        pmd_cachep = kmem_cache_create("pmd_cache",
                                       PTRS_PER_PMD * (1<<PTE_MAGNITUDE),

While I can confirm your patch fixes the

    WARNING: CPU: 0 PID: 1 at mm/slub.c:2412
___slab_alloc.constprop.34+0x196/0x288

seen since commit 128227e7fe4087b6 ("slab: __GFP_ZERO is incompatible with a constructor"), I'm not 100% sure this is the correct fix.

swapper_pg_dir[] does have two non-zero entries (768 and 895), which were copied by the constructor. Perhaps SH does have a need for these two entries, and thus for the constructor?

Quoted 3 lines · Geert Uytterhoeven

Hi Sato-san,

CC Rob , Willy, linux-arch

Thanks!

Quoted 24 lines

Quoted 8 lines

arch/sh/mm/pgtable.c +0 -8

-void pgd_ctor(void *x)
-{
-       pgd_t *pgd = x;
-
-       memcpy(pgd + USER_PTRS_PER_PGD,
-              swapper_pg_dir + USER_PTRS_PER_PGD,
-              (PTRS_PER_PGD - USER_PTRS_PER_PGD) * sizeof(pgd_t));
-}

arch/sh/mm/pgtable.c static struct kmem_cache *pgd_cachep; · line 19

Quoted 10 lines

arch/sh/mm/pgtable.c +1 -2

-
 void pgtable_cache_init(void)
 {
        pgd_cachep = kmem_cache_create("pgd_cache",
                                       PTRS_PER_PGD * (1<<PTE_MAGNITUDE),
-                                      PAGE_SIZE, SLAB_PANIC, pgd_ctor);
+                                      PAGE_SIZE, SLAB_PANIC, NULL);
 #if PAGETABLE_LEVELS > 2
        pmd_cachep = kmem_cache_create("pmd_cache",
                                       PTRS_PER_PMD * (1<<PTE_MAGNITUDE),

While I can confirm your patch fixes the

    WARNING: CPU: 0 PID: 1 at mm/slub.c:2412
___slab_alloc.constprop.34+0x196/0x288

Quoted 6 lines

seen since commit 128227e7fe4087b6 ("slab: __GFP_ZERO is incompatible with a constructor"), I'm not 100% sure this is the correct fix.

swapper_pg_dir[] does have two non-zero entries (768 and 895), which were copied by the constructor. Perhaps SH does have a need for these two entries, and thus for the constructor?

__GFP_ZERO overrode the constructor. That is, before 128227e7fe40, if you specified both a constructor and __GFP_ZERO, first the slab code would invoke the constructor, then it would zero the allocation. So this patch is preserving the existing behaviour. Whether the existing behaviour is correct or not, I cannot say.

Quoted 5 lines · Matthew Wilcox

__GFP_ZERO overrode the constructor. That is, before 128227e7fe40, if you specified both a constructor and __GFP_ZERO, first the slab code would invoke the constructor, then it would zero the allocation. So this patch is preserving the existing behaviour. Whether the existing behaviour is correct or not, I cannot say.

Then I think we should really try to figure out whether this is a buried bug before deleting the evidence of it...