| Message ID | 20180731051519.101249-1-ysato@users.sourceforge.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sh: remove unneeded constructor. | expand |
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 -- 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
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
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. -- 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
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 -- 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
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! -- 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
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. -- 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
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
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) -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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
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 -- 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
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
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
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.
diff --git a/arch/sh/mm/pgtable.c b/arch/sh/mm/pgtable.c index 5c8f9247c3c2..4b8be7525fba 100644 --- 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),
pgd_cache specifies __GFP_ZERO when allocating. This constructor is meaningless. Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> --- arch/sh/mm/pgtable.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)