| Message ID | 1892b37d1fd9a4ed39e76c4b999b6556077201c0.1568355752.git.christophe.leroy@c-s.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm/pgtable/debug: Fix test validating architecture page table helpers | expand |
On 09/13/2019 11:53 AM, Christophe Leroy wrote: > Fix build failure on powerpc. > > Fix preemption imbalance. > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > mm/arch_pgtable_test.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/mm/arch_pgtable_test.c b/mm/arch_pgtable_test.c > index 8b4a92756ad8..f2b3c9ec35fa 100644 > --- a/mm/arch_pgtable_test.c > +++ b/mm/arch_pgtable_test.c > @@ -24,6 +24,7 @@ > #include <linux/swap.h> > #include <linux/swapops.h> > #include <linux/sched/mm.h> > +#include <linux/highmem.h> This is okay. > #include <asm/pgalloc.h> > #include <asm/pgtable.h> > > @@ -400,6 +401,8 @@ static int __init arch_pgtable_tests_init(void) > p4d_clear_tests(p4dp); > pgd_clear_tests(mm, pgdp); > > + pte_unmap(ptep); > + Now the preemption imbalance via pte_alloc_map() path i.e pte_alloc_map() -> pte_offset_map() -> kmap_atomic() Is not this very much powerpc 32 specific or this will be applicable for all platform which uses kmap_XXX() to map high memory ?
Le 13/09/2019 à 08:58, Anshuman Khandual a écrit : > On 09/13/2019 11:53 AM, Christophe Leroy wrote: >> Fix build failure on powerpc. >> >> Fix preemption imbalance. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >> --- >> mm/arch_pgtable_test.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/mm/arch_pgtable_test.c b/mm/arch_pgtable_test.c >> index 8b4a92756ad8..f2b3c9ec35fa 100644 >> --- a/mm/arch_pgtable_test.c >> +++ b/mm/arch_pgtable_test.c >> @@ -24,6 +24,7 @@ >> #include <linux/swap.h> >> #include <linux/swapops.h> >> #include <linux/sched/mm.h> >> +#include <linux/highmem.h> > > This is okay. > >> #include <asm/pgalloc.h> >> #include <asm/pgtable.h> >> >> @@ -400,6 +401,8 @@ static int __init arch_pgtable_tests_init(void) >> p4d_clear_tests(p4dp); >> pgd_clear_tests(mm, pgdp); >> >> + pte_unmap(ptep); >> + > > Now the preemption imbalance via pte_alloc_map() path i.e > > pte_alloc_map() -> pte_offset_map() -> kmap_atomic() > > Is not this very much powerpc 32 specific or this will be applicable > for all platform which uses kmap_XXX() to map high memory ? > See https://elixir.bootlin.com/linux/v5.3-rc8/source/include/linux/highmem.h#L91 I think it applies at least to all arches using the generic implementation. Applies also to arm: https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/arm/mm/highmem.c#L52 Applies also to mips: https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/mips/mm/highmem.c#L47 Same on sparc: https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/sparc/mm/highmem.c#L52 Same on x86: https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/mm/highmem_32.c#L34 I have not checked others, but I guess it is like that for all. Christophe
Le 13/09/2019 à 09:03, Christophe Leroy a écrit : > > > Le 13/09/2019 à 08:58, Anshuman Khandual a écrit : >> On 09/13/2019 11:53 AM, Christophe Leroy wrote: >>> Fix build failure on powerpc. >>> >>> Fix preemption imbalance. >>> >>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >>> --- >>> mm/arch_pgtable_test.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/mm/arch_pgtable_test.c b/mm/arch_pgtable_test.c >>> index 8b4a92756ad8..f2b3c9ec35fa 100644 >>> --- a/mm/arch_pgtable_test.c >>> +++ b/mm/arch_pgtable_test.c >>> @@ -24,6 +24,7 @@ >>> #include <linux/swap.h> >>> #include <linux/swapops.h> >>> #include <linux/sched/mm.h> >>> +#include <linux/highmem.h> >> >> This is okay. >> >>> #include <asm/pgalloc.h> >>> #include <asm/pgtable.h> >>> @@ -400,6 +401,8 @@ static int __init arch_pgtable_tests_init(void) >>> p4d_clear_tests(p4dp); >>> pgd_clear_tests(mm, pgdp); >>> + pte_unmap(ptep); >>> + >> >> Now the preemption imbalance via pte_alloc_map() path i.e >> >> pte_alloc_map() -> pte_offset_map() -> kmap_atomic() >> >> Is not this very much powerpc 32 specific or this will be applicable >> for all platform which uses kmap_XXX() to map high memory ? >> > > See > https://elixir.bootlin.com/linux/v5.3-rc8/source/include/linux/highmem.h#L91 > > > I think it applies at least to all arches using the generic implementation. > > Applies also to arm: > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/arm/mm/highmem.c#L52 > > Applies also to mips: > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/mips/mm/highmem.c#L47 > > Same on sparc: > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/sparc/mm/highmem.c#L52 > > > Same on x86: > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/mm/highmem_32.c#L34 > > > I have not checked others, but I guess it is like that for all. > Seems like I answered too quickly. All kmap_atomic() do preempt_disable(), but not all pte_alloc_map() call kmap_atomic(). However, for instance ARM does: https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/arm/include/asm/pgtable.h#L200 And X86 as well: https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/include/asm/pgtable_32.h#L51 Microblaze also: https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/microblaze/include/asm/pgtable.h#L495 Christophe
On 09/13/2019 12:41 PM, Christophe Leroy wrote: > > > Le 13/09/2019 à 09:03, Christophe Leroy a écrit : >> >> >> Le 13/09/2019 à 08:58, Anshuman Khandual a écrit : >>> On 09/13/2019 11:53 AM, Christophe Leroy wrote: >>>> Fix build failure on powerpc. >>>> >>>> Fix preemption imbalance. >>>> >>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >>>> --- >>>> mm/arch_pgtable_test.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/mm/arch_pgtable_test.c b/mm/arch_pgtable_test.c >>>> index 8b4a92756ad8..f2b3c9ec35fa 100644 >>>> --- a/mm/arch_pgtable_test.c >>>> +++ b/mm/arch_pgtable_test.c >>>> @@ -24,6 +24,7 @@ >>>> #include <linux/swap.h> >>>> #include <linux/swapops.h> >>>> #include <linux/sched/mm.h> >>>> +#include <linux/highmem.h> >>> >>> This is okay. >>> >>>> #include <asm/pgalloc.h> >>>> #include <asm/pgtable.h> >>>> @@ -400,6 +401,8 @@ static int __init arch_pgtable_tests_init(void) >>>> p4d_clear_tests(p4dp); >>>> pgd_clear_tests(mm, pgdp); >>>> + pte_unmap(ptep); >>>> + >>> >>> Now the preemption imbalance via pte_alloc_map() path i.e >>> >>> pte_alloc_map() -> pte_offset_map() -> kmap_atomic() >>> >>> Is not this very much powerpc 32 specific or this will be applicable >>> for all platform which uses kmap_XXX() to map high memory ? >>> >> >> See https://elixir.bootlin.com/linux/v5.3-rc8/source/include/linux/highmem.h#L91 >> >> I think it applies at least to all arches using the generic implementation. >> >> Applies also to arm: >> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/arm/mm/highmem.c#L52 >> >> Applies also to mips: >> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/mips/mm/highmem.c#L47 >> >> Same on sparc: >> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/sparc/mm/highmem.c#L52 >> >> Same on x86: >> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/mm/highmem_32.c#L34 >> >> I have not checked others, but I guess it is like that for all. >> > > > Seems like I answered too quickly. All kmap_atomic() do preempt_disable(), but not all pte_alloc_map() call kmap_atomic(). > > However, for instance ARM does: > > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/arm/include/asm/pgtable.h#L200 > > And X86 as well: > > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/include/asm/pgtable_32.h#L51 > > Microblaze also: > > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/microblaze/include/asm/pgtable.h#L495 All the above platforms checks out to be using k[un]map_atomic(). I am wondering whether any of the intermediate levels will have similar problems on any these 32 bit platforms or any other platforms which might be using generic k[un]map_atomic(). There can be many permutations here. p4dp = p4d_alloc(mm, pgdp, vaddr); pudp = pud_alloc(mm, p4dp, vaddr); pmdp = pmd_alloc(mm, pudp, vaddr); Otherwise pte_alloc_map()/pte_unmap() looks good enough which will atleast take care of a known failure.
On Fri, Sep 13, 2019 at 02:12:45PM +0530, Anshuman Khandual wrote: > > > On 09/13/2019 12:41 PM, Christophe Leroy wrote: > > > > > > Le 13/09/2019 à 09:03, Christophe Leroy a écrit : > >> > >> > >> Le 13/09/2019 à 08:58, Anshuman Khandual a écrit : > >>> On 09/13/2019 11:53 AM, Christophe Leroy wrote: > >>>> Fix build failure on powerpc. > >>>> > >>>> Fix preemption imbalance. > >>>> > >>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > >>>> --- > >>>> mm/arch_pgtable_test.c | 3 +++ > >>>> 1 file changed, 3 insertions(+) > >>>> > >>>> diff --git a/mm/arch_pgtable_test.c b/mm/arch_pgtable_test.c > >>>> index 8b4a92756ad8..f2b3c9ec35fa 100644 > >>>> --- a/mm/arch_pgtable_test.c > >>>> +++ b/mm/arch_pgtable_test.c > >>>> @@ -24,6 +24,7 @@ > >>>> #include <linux/swap.h> > >>>> #include <linux/swapops.h> > >>>> #include <linux/sched/mm.h> > >>>> +#include <linux/highmem.h> > >>> > >>> This is okay. > >>> > >>>> #include <asm/pgalloc.h> > >>>> #include <asm/pgtable.h> > >>>> @@ -400,6 +401,8 @@ static int __init arch_pgtable_tests_init(void) > >>>> p4d_clear_tests(p4dp); > >>>> pgd_clear_tests(mm, pgdp); > >>>> + pte_unmap(ptep); > >>>> + > >>> > >>> Now the preemption imbalance via pte_alloc_map() path i.e > >>> > >>> pte_alloc_map() -> pte_offset_map() -> kmap_atomic() > >>> > >>> Is not this very much powerpc 32 specific or this will be applicable > >>> for all platform which uses kmap_XXX() to map high memory ? > >>> > >> > >> See https://elixir.bootlin.com/linux/v5.3-rc8/source/include/linux/highmem.h#L91 > >> > >> I think it applies at least to all arches using the generic implementation. > >> > >> Applies also to arm: > >> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/arm/mm/highmem.c#L52 > >> > >> Applies also to mips: > >> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/mips/mm/highmem.c#L47 > >> > >> Same on sparc: > >> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/sparc/mm/highmem.c#L52 > >> > >> Same on x86: > >> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/mm/highmem_32.c#L34 > >> > >> I have not checked others, but I guess it is like that for all. > >> > > > > > > Seems like I answered too quickly. All kmap_atomic() do preempt_disable(), but not all pte_alloc_map() call kmap_atomic(). > > > > However, for instance ARM does: > > > > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/arm/include/asm/pgtable.h#L200 > > > > And X86 as well: > > > > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/include/asm/pgtable_32.h#L51 > > > > Microblaze also: > > > > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/microblaze/include/asm/pgtable.h#L495 > > All the above platforms checks out to be using k[un]map_atomic(). I am wondering whether > any of the intermediate levels will have similar problems on any these 32 bit platforms > or any other platforms which might be using generic k[un]map_atomic(). No. Kernel only allocates pte page table from highmem. All other page tables are always visible in kernel address space.
On 09/13/2019 11:53 AM, Christophe Leroy wrote: > Fix build failure on powerpc. > > Fix preemption imbalance. > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > mm/arch_pgtable_test.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/mm/arch_pgtable_test.c b/mm/arch_pgtable_test.c > index 8b4a92756ad8..f2b3c9ec35fa 100644 > --- a/mm/arch_pgtable_test.c > +++ b/mm/arch_pgtable_test.c > @@ -24,6 +24,7 @@ > #include <linux/swap.h> > #include <linux/swapops.h> > #include <linux/sched/mm.h> > +#include <linux/highmem.h> > #include <asm/pgalloc.h> > #include <asm/pgtable.h> > > @@ -400,6 +401,8 @@ static int __init arch_pgtable_tests_init(void) > p4d_clear_tests(p4dp); > pgd_clear_tests(mm, pgdp); > > + pte_unmap(ptep); > + > pmd_populate_tests(mm, pmdp, saved_ptep); > pud_populate_tests(mm, pudp, saved_pmdp); > p4d_populate_tests(mm, p4dp, saved_pudp); > Hello Christophe, I am planning to fold this fix into the current patch and retain your Signed-off-by. Are you okay with it ? - Anshuman
Le 18/09/2019 à 09:32, Anshuman Khandual a écrit : > > > On 09/13/2019 11:53 AM, Christophe Leroy wrote: >> Fix build failure on powerpc. >> >> Fix preemption imbalance. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> >> --- >> mm/arch_pgtable_test.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/mm/arch_pgtable_test.c b/mm/arch_pgtable_test.c >> index 8b4a92756ad8..f2b3c9ec35fa 100644 >> --- a/mm/arch_pgtable_test.c >> +++ b/mm/arch_pgtable_test.c >> @@ -24,6 +24,7 @@ >> #include <linux/swap.h> >> #include <linux/swapops.h> >> #include <linux/sched/mm.h> >> +#include <linux/highmem.h> >> #include <asm/pgalloc.h> >> #include <asm/pgtable.h> >> >> @@ -400,6 +401,8 @@ static int __init arch_pgtable_tests_init(void) >> p4d_clear_tests(p4dp); >> pgd_clear_tests(mm, pgdp); >> >> + pte_unmap(ptep); >> + >> pmd_populate_tests(mm, pmdp, saved_ptep); >> pud_populate_tests(mm, pudp, saved_pmdp); >> p4d_populate_tests(mm, p4dp, saved_pudp); >> > > Hello Christophe, > > I am planning to fold this fix into the current patch and retain your > Signed-off-by. Are you okay with it ? > No problem, do whatever is convenient for you. You can keep the signed-off-by, or use tested-by: as I tested it on PPC32. Christophe
diff --git a/mm/arch_pgtable_test.c b/mm/arch_pgtable_test.c index 8b4a92756ad8..f2b3c9ec35fa 100644 --- a/mm/arch_pgtable_test.c +++ b/mm/arch_pgtable_test.c @@ -24,6 +24,7 @@ #include <linux/swap.h> #include <linux/swapops.h> #include <linux/sched/mm.h> +#include <linux/highmem.h> #include <asm/pgalloc.h> #include <asm/pgtable.h> @@ -400,6 +401,8 @@ static int __init arch_pgtable_tests_init(void) p4d_clear_tests(p4dp); pgd_clear_tests(mm, pgdp); + pte_unmap(ptep); + pmd_populate_tests(mm, pmdp, saved_ptep); pud_populate_tests(mm, pudp, saved_pmdp); p4d_populate_tests(mm, p4dp, saved_pudp);
Fix build failure on powerpc. Fix preemption imbalance. Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> --- mm/arch_pgtable_test.c | 3 +++ 1 file changed, 3 insertions(+)