Re: [PATCH] sh: remove unneeded constructor.

Re: [PATCH] sh: remove unneeded constructor.

[Geert Uytterhoeven]

Hi Sato-san,

CC Rob , Willy, linux-arch

On Tue, Jul 31, 2018 at 7:15 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> pgd_cache specifies __GFP_ZERO when allocating.
> This constructor is meaningless.
>
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>

Thanks for your patch!

> --- a/arch/sh/mm/pgtable.c
> +++ b/arch/sh/mm/pgtable.c
> @@ -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));
> -}
> -
>  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 -- geert@linux-m68k.org

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

Re: [PATCH] sh: remove unneeded constructor.

[Yoshinori Sato]

On Wed, 01 Aug 2018 16:42:02 +0900,
Geert Uytterhoeven wrote:
> 
> Hi Sato-san,
> 
> CC Rob , Willy, linux-arch
> 
> On Tue, Jul 31, 2018 at 7:15 AM Yoshinori Sato
> <ysato@users.sourceforge.jp> wrote:
> > pgd_cache specifies __GFP_ZERO when allocating.
> > This constructor is meaningless.
> >
> > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> 
> Thanks for your patch!
> 
> > --- a/arch/sh/mm/pgtable.c
> > +++ b/arch/sh/mm/pgtable.c
> > @@ -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));
> > -}
> > -
> >  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.

I tried it in myself, but with this fix I will not get a warning.
I do not think that it is related.

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

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.

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Yosinori Sato

Re: [PATCH] sh: remove unneeded constructor.

[Matthew Wilcox]

On Wed, Aug 01, 2018 at 08:13:26PM +0900, Yoshinori Sato wrote:
> 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):

diff --git a/arch/sh/mm/pgtable.c b/arch/sh/mm/pgtable.c
index 5c8f9247c3c2..7c63aa359c7d 100644
--- a/arch/sh/mm/pgtable.c
+++ b/arch/sh/mm/pgtable.c
@@ -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.

Re: [PATCH] sh: remove unneeded constructor.

[Geert Uytterhoeven]

Hi Willy,

On Sat, Aug 4, 2018 at 12:36 PM Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, Aug 01, 2018 at 08:13:26PM +0900, Yoshinori Sato wrote:
> > 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):
>
> diff --git a/arch/sh/mm/pgtable.c b/arch/sh/mm/pgtable.c
> index 5c8f9247c3c2..7c63aa359c7d 100644
> --- a/arch/sh/mm/pgtable.c
> +++ b/arch/sh/mm/pgtable.c
> @@ -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.

Works equally well on qemu emulating rts7751r2d.

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

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] sh: remove unneeded constructor.

[Matthew Wilcox]

On Sat, Aug 04, 2018 at 12:47:08PM +0200, Geert Uytterhoeven wrote:
> 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
@@ -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)
@@ -48,7 +47,7 @@ void pud_populate(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
 
 pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
 {
-       return kmem_cache_alloc(pmd_cachep, PGALLOC_GFP);
+       return kmem_cache_alloc(pmd_cachep, GFP_KERNEL | __GFP_ZERO);
 }
 
 void pmd_free(struct mm_struct *mm, pmd_t *pmd)

Re: [PATCH] sh: remove unneeded constructor.

[Rob Landley]

On 08/04/2018 05:51 AM, Matthew Wilcox wrote:
> On Sat, Aug 04, 2018 at 12:47:08PM +0200, Geert Uytterhoeven wrote:
>> 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 know the j-core project is taking a different 64 bit approach, much more like
x86-64's mode bit than itanium two completely different instruction sets, but
last I heard they're just now testing the j32 with-mmu bitstreams so it'll be a
while before that's relevant. :)

Rob

Re: [PATCH] sh: remove unneeded constructor.

[Rich Felker]

On Sun, Aug 05, 2018 at 10:54:56AM -0500, Rob Landley wrote:
> On 08/04/2018 05:51 AM, Matthew Wilcox wrote:
> > On Sat, Aug 04, 2018 at 12:47:08PM +0200, Geert Uytterhoeven wrote:
> >> 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.

Rich

Re: [PATCH] sh: remove unneeded constructor.

[Rob Landley]

On 08/05/2018 11:32 AM, Rich Felker wrote:
> On Sun, Aug 05, 2018 at 10:54:56AM -0500, Rob Landley wrote:
>> On 08/04/2018 05:51 AM, Matthew Wilcox wrote:
>>> On Sat, Aug 04, 2018 at 12:47:08PM +0200, Geert Uytterhoeven wrote:
>>>> 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.

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...)

Rob

Re: [PATCH] sh: remove unneeded constructor.

[Rich Felker]

On Sun, Aug 05, 2018 at 05:48:52PM -0500, Rob Landley wrote:
> On 08/05/2018 11:32 AM, Rich Felker wrote:
> > On Sun, Aug 05, 2018 at 10:54:56AM -0500, Rob Landley wrote:
> >> On 08/04/2018 05:51 AM, Matthew Wilcox wrote:
> >>> On Sat, Aug 04, 2018 at 12:47:08PM +0200, Geert Uytterhoeven wrote:
> >>>> 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.
> 
> 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".

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

Rich

Re: [PATCH] sh: remove unneeded constructor.

[Geert Uytterhoeven]

Hi Rich,

CC Arnd

On Mon, Aug 6, 2018 at 1:34 AM Rich Felker <dalias@libc.org> wrote:
> On Sun, Aug 05, 2018 at 05:48:52PM -0500, Rob Landley wrote:
> > On 08/05/2018 11:32 AM, Rich Felker wrote:
> > > On Sun, Aug 05, 2018 at 10:54:56AM -0500, Rob Landley wrote:
> > >> On 08/04/2018 05:51 AM, Matthew Wilcox wrote:
> > >>> On Sat, Aug 04, 2018 at 12:47:08PM +0200, Geert Uytterhoeven wrote:
> > >>>> 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.
> >
> > 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".
>
> > 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.

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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] sh: remove unneeded constructor.

[Arnd Bergmann]

On Mon, Aug 6, 2018 at 8:57 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, Aug 6, 2018 at 1:34 AM Rich Felker <dalias@libc.org> wrote:
> > On Sun, Aug 05, 2018 at 05:48:52PM -0500, Rob Landley wrote:
> > > 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.
> >
> > 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.

I suspect arch/sh is in a similar state to what arch/arm was in when we added
arch/arm64, where 90% of the code is there to deal with concepts that no longer
apply to the new code base, in particular board support for pre-DT machines.

Starting a new arch/j64 would then make it look more like a clean arch/riscv/ or
arch/nds32, which are the most recently added ones. However, if you do
that, you should really start from scratch instead of copying the existing
arch code, which was the mistake that arch/arm64 did.

       Arnd

Re: [PATCH] sh: remove unneeded constructor.

[Rob Landley]

On 08/06/2018 01:57 AM, Geert Uytterhoeven wrote:
> 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.

At $DAYJOB I'm working on a sh7760 board with platform data (the open source
release of the linux patches for which looks like it'll miss this merge window
but might hit next one; they only want to run everything through the lawyers
once so legal clearance waits until we're done fiddling).

The device tree people break platform data support because they only test device
tree, and then refuse patches to make the existing platform data work again. For
example, I have this bookmarked:

  http://lkml.iu.edu/hypermail/linux/kernel/1807.0/05252.html

In case I ever have time to go back and look at it, but I've moved on to other
things already. (My current "circle back to poke at it as I have time" is trying
to get DMAEngine working for sh7760 with the smc91x.c.) The fix I sent upstream
for the LEDs is the fix we're shipping with. It's the simple, obvious, "make it
work exactly like it used to work", and they went "nah, we don't want to do
that, do something else that will require changing the platform data that worked
before our commit". (See longish thread, above. The reply I bookmarked was the
polite one.)

I'm aware there's a problem regression testing old hardware, which limits
conversion of that old hardware to device tree. But the people who _have_ old
hardware continue to run old kernels because upgrading to current kernels
doesn't _work_. And then when we make it work, our patches to make it work get
rejected presumably on the grounds that the device tree people want to make not
having already migrated everything to systemd painful. So current kernels won't
"just work" for the _next_ person either, and nobody upgrades, and then if
somebody tries their hand at converting a board to device tree there's no
testers. Or if there are they go "it didn't boot" because of some unrelated
regression elsewhere in the code because the kernel they're using (and still
maintaining in-house) is years old...

This is something like the eighth company I've seen that at. :P

Rob

Re: [PATCH] sh: remove unneeded constructor.

[Rich Felker]

On Sat, Aug 04, 2018 at 03:51:49AM -0700, Matthew Wilcox wrote:
> On Sat, Aug 04, 2018 at 12:47:08PM +0200, Geert Uytterhoeven wrote:
> > 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
> @@ -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)
> @@ -48,7 +47,7 @@ void pud_populate(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
>  
>  pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
>  {
> -       return kmem_cache_alloc(pmd_cachep, PGALLOC_GFP);
> +       return kmem_cache_alloc(pmd_cachep, GFP_KERNEL | __GFP_ZERO);
>  }
>  
>  void pmd_free(struct mm_struct *mm, pmd_t *pmd)
> 
> --

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?

Rich

Re: [PATCH] sh: remove unneeded constructor.

[Rich Felker]

On Sat, Aug 04, 2018 at 03:51:49AM -0700, Matthew Wilcox wrote:
> On Sat, Aug 04, 2018 at 12:47:08PM +0200, Geert Uytterhoeven wrote:
> > 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
> @@ -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)
> @@ -48,7 +47,7 @@ void pud_populate(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
>  
>  pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
>  {
> -       return kmem_cache_alloc(pmd_cachep, PGALLOC_GFP);
> +       return kmem_cache_alloc(pmd_cachep, GFP_KERNEL | __GFP_ZERO);
>  }
>  
>  void pmd_free(struct mm_struct *mm, pmd_t *pmd)
> 
> --

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?

Rich

Re: [PATCH] sh: remove unneeded constructor.

[Matthew Wilcox]

On Sat, Jun 06, 2020 at 12:32:45PM -0400, Rich Felker wrote:
> 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?

August 2018?  Things certainly move quickly in SH land ;-)  I'll send
that along.

Re: [PATCH] sh: remove unneeded constructor.

[Matthew Wilcox]

On Sat, Jun 06, 2020 at 12:32:45PM -0400, Rich Felker wrote:
> 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?

August 2018?  Things certainly move quickly in SH land ;-)  I'll send
that along.

Re: [PATCH] sh: remove unneeded constructor.

[Matthew Wilcox]

On Wed, Aug 01, 2018 at 09:42:02AM +0200, Geert Uytterhoeven wrote:
> Hi Sato-san,
> 
> CC Rob , Willy, linux-arch

Thanks!

> > -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));
> > -}
> > -
> >  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?

__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.

Re: [PATCH] sh: remove unneeded constructor.

[Rich Felker]

On Wed, Aug 01, 2018 at 04:20:19AM -0700, Matthew Wilcox wrote:
> On Wed, Aug 01, 2018 at 09:42:02AM +0200, Geert Uytterhoeven wrote:
> > Hi Sato-san,
> > 
> > CC Rob , Willy, linux-arch
> 
> Thanks!
> 
> > > -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));
> > > -}
> > > -
> > >  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?
> 
> __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...

Rich

Re: [PATCH] sh: remove unneeded constructor.

[Matthew Wilcox]

On Wed, Aug 01, 2018 at 07:02:05PM -0400, Rich Felker wrote:
> On Wed, Aug 01, 2018 at 04:20:19AM -0700, Matthew Wilcox wrote:
> > __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...

Archaeology suggests the bug was introduced in commit 2a5eacca85d3 ("sh:
Move page table allocation out of line") in 2009.  Previous code:

-       pgd = kzalloc(sizeof(*pgd) * PTRS_PER_PGD, GFP_KERNEL | __GFP_REPEAT);
-
-       for (i = USER_PTRS_PER_PGD; i < PTRS_PER_PGD; i++)
-               pgd[i] = swapper_pg_dir[i];

Replacement code:

+#define PGALLOC_GFP GFP_KERNEL | __GFP_REPEAT | __GFP_ZERO

+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));
+}

+       pgd_cachep = kmem_cache_create("pgd_cache",
+                                      PTRS_PER_PGD * (1<<PTE_MAGNITUDE),
+                                      PAGE_SIZE, SLAB_PANIC, pgd_ctor);

+pgd_t *pgd_alloc(struct mm_struct *mm)
+{
+       return kmem_cache_alloc(pgd_cachep, PGALLOC_GFP);

It clearly doesn't cause any bugs to zero the PGD entries (... or somebody
would have noticed since 2009?), but I suspect it causes various PMD
entries to not be shared.

I'm really happy we decided to introduce this check.  It caught a really
old and completely unrelated bug!