diff mbox series

[v2] sh: Update kmem_cache_flag.

Message ID 20180802122136.127494-1-ysato@users.sourceforge.jp (mailing list archive)
State New, archived
Headers show
Series [v2] sh: Update kmem_cache_flag. | expand

Commit Message

Yoshinori Sato Aug. 2, 2018, 12:21 p.m. UTC 
__GFP_ZERO and the constructor are exclusive relationships,
so it is incorrect to specify them at the same time.
---
 arch/sh/mm/pgtable.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Geert Uytterhoeven Aug. 2, 2018, 12:50 p.m. UTC | #1 
Hi Sato-san,

On Thu, Aug 2, 2018 at 2:22 PM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> __GFP_ZERO and the constructor are exclusive relationships,
> so it is incorrect to specify them at the same time.

Thanks for your patch!

> --- a/arch/sh/mm/pgtable.c
> +++ 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;
> @@ -32,7 +30,9 @@ void pgtable_cache_init(void)
>
>  pgd_t *pgd_alloc(struct mm_struct *mm)
>  {
> -       return kmem_cache_alloc(pgd_cachep, PGALLOC_GFP);
> +       pgd_t *pgd = kmem_cache_alloc(pgd_cachep, GFP_KERNEL);
> +       memset(pgd, 0, kmem_cache_size(pgd_cachep));

Are you sure the above line is needed?
As kmem_cache_size(pgd_cachep) == 4096, this clears the whole pgd again,
undoing the constructor's work.

> +       return pgd;
>  }
>
>  void pgd_free(struct mm_struct *mm, pgd_t *pgd)

Gr{oetje,eeting}s,

                        Geert
Yoshinori Sato Aug. 2, 2018, 2:03 p.m. UTC | #2 
On Thu, 02 Aug 2018 21:50:00 +0900,
Geert Uytterhoeven wrote:
> 
> Hi Sato-san,
> 
> On Thu, Aug 2, 2018 at 2:22 PM Yoshinori Sato
> <ysato@users.sourceforge.jp> wrote:
> > __GFP_ZERO and the constructor are exclusive relationships,
> > so it is incorrect to specify them at the same time.
> 
> Thanks for your patch!
> 
> > --- a/arch/sh/mm/pgtable.c
> > +++ 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;
> > @@ -32,7 +30,9 @@ void pgtable_cache_init(void)
> >
> >  pgd_t *pgd_alloc(struct mm_struct *mm)
> >  {
> > -       return kmem_cache_alloc(pgd_cachep, PGALLOC_GFP);
> > +       pgd_t *pgd = kmem_cache_alloc(pgd_cachep, GFP_KERNEL);
> > +       memset(pgd, 0, kmem_cache_size(pgd_cachep));
> 
> Are you sure the above line is needed?
> As kmem_cache_size(pgd_cachep) == 4096, this clears the whole pgd again,
> undoing the constructor's work.

If there is no such line, panic with page fault.
I have not identified the cause, but there seems
to be a part that expects 0 somewhere.

> > +       return pgd;
> >  }
> >
> >  void pgd_free(struct mm_struct *mm, pgd_t *pgd)
> 
> 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
Geert Uytterhoeven Aug. 2, 2018, 2:11 p.m. UTC | #3 
Hi Sato-san,

On Thu, Aug 2, 2018 at 4:03 PM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> On Thu, 02 Aug 2018 21:50:00 +0900,
> Geert Uytterhoeven wrote:
> > On Thu, Aug 2, 2018 at 2:22 PM Yoshinori Sato
> > <ysato@users.sourceforge.jp> wrote:
> > > __GFP_ZERO and the constructor are exclusive relationships,
> > > so it is incorrect to specify them at the same time.
> >
> > Thanks for your patch!
> >
> > > --- a/arch/sh/mm/pgtable.c
> > > +++ 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;
> > > @@ -32,7 +30,9 @@ void pgtable_cache_init(void)
> > >
> > >  pgd_t *pgd_alloc(struct mm_struct *mm)
> > >  {
> > > -       return kmem_cache_alloc(pgd_cachep, PGALLOC_GFP);
> > > +       pgd_t *pgd = kmem_cache_alloc(pgd_cachep, GFP_KERNEL);
> > > +       memset(pgd, 0, kmem_cache_size(pgd_cachep));
> >
> > Are you sure the above line is needed?
> > As kmem_cache_size(pgd_cachep) == 4096, this clears the whole pgd again,
> > undoing the constructor's work.
>
> If there is no such line, panic with page fault.
> I have not identified the cause, but there seems
> to be a part that expects 0 somewhere.

On QEMU emulating rts7751r2d it works fine.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/arch/sh/mm/pgtable.c b/arch/sh/mm/pgtable.c
index 5c8f9247c3c2..5a54c2bce2de 100644
--- a/arch/sh/mm/pgtable.c
+++ 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;
@@ -32,7 +30,9 @@  void pgtable_cache_init(void)
 
 pgd_t *pgd_alloc(struct mm_struct *mm)
 {
-	return kmem_cache_alloc(pgd_cachep, PGALLOC_GFP);
+	pgd_t *pgd = kmem_cache_alloc(pgd_cachep, GFP_KERNEL);
+	memset(pgd, 0, kmem_cache_size(pgd_cachep));
+	return pgd;
 }
 
 void pgd_free(struct mm_struct *mm, pgd_t *pgd)
@@ -48,7 +48,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)