Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
On 10/25/2019 02:22 PM, Christophe Leroy wrote:
>
>
> Le 25/10/2019 à 10:24, Anshuman Khandual a écrit :
>>
>>
>> On 10/25/2019 12:41 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 25/10/2019 à 07:52, Qian Cai a écrit :
> On Oct 24, 2019, at 11:45 PM, Anshuman Khandual
> wrote:
>
> Nothing specific. But just tested this with x86 defconfig with relevant
> configs
> which are required for this test. Not sure if it involved W=1.
No, it will not. It needs to run like,
make W=1 -j 64 2>/tmp/warns
>>>
>>> Are we talking about this peace of code ?
>>>
>>> +static unsigned long __init get_random_vaddr(void)
>>> +{
>>> + unsigned long random_vaddr, random_pages, total_user_pages;
>>> +
>>> + total_user_pages = (TASK_SIZE - FIRST_USER_ADDRESS) / PAGE_SIZE;
>>> +
>>> + random_pages = get_random_long() % total_user_pages;
>>> + random_vaddr = FIRST_USER_ADDRESS + random_pages * PAGE_SIZE;
>>> +
>>> + WARN_ON((random_vaddr > TASK_SIZE) ||
>>> + (random_vaddr < FIRST_USER_ADDRESS));
>>> + return random_vaddr;
>>> +}
>>> +
>>>
>>> ramdom_vaddr is unsigned,
>>> random_pages is unsigned and lower than total_user_pages
>>>
>>> So the max value random_vaddr can get is FIRST_USER_ADDRESS + ((TASK_SIZE -
>>> FIRST_USER_ADDRESS - 1) / PAGE_SIZE) * PAGE_SIZE = TASK_SIZE - 1
>>> And the min value random_vaddr can get is FIRST_USER_ADDRESS (that's when
>>> random_pages = 0)
>>
>> That's right.
>>
>>>
>>> So the WARN_ON() is just unneeded, isn't it ?
>>
>> It is just a sanity check on possible vaddr values before it's corresponding
>> page table mappings could be created. If it's worth to drop this in favor of
>> avoiding these unwanted warning messages on x86, will go ahead with it as it
>> is not super important.
>>
>
> But you are checking what ? That the compiler does calculation correctly or
> what ?
IIRC, probably this was for later if and when the vaddr calculation becomes
dependent on other factors rather than this simple arithmetic involving start
and end of process address space on a platform.
> As mentionned just above, based on the calculation done, what you are testing
> cannot happen, so I'm having a hard time understanding what kind of sanity
> check it can be.
You are right.
>
> Can you give an exemple of a situation which could trigger the warning ?
I was mistaken. We dont need those checks for now, hence will drop them next
time.
>
> Christophe
>
Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
Le 25/10/2019 à 10:24, Anshuman Khandual a écrit :
On 10/25/2019 12:41 PM, Christophe Leroy wrote:
Le 25/10/2019 à 07:52, Qian Cai a écrit :
On Oct 24, 2019, at 11:45 PM, Anshuman Khandual
wrote:
Nothing specific. But just tested this with x86 defconfig with relevant configs
which are required for this test. Not sure if it involved W=1.
No, it will not. It needs to run like,
make W=1 -j 64 2>/tmp/warns
Are we talking about this peace of code ?
+static unsigned long __init get_random_vaddr(void)
+{
+ unsigned long random_vaddr, random_pages, total_user_pages;
+
+ total_user_pages = (TASK_SIZE - FIRST_USER_ADDRESS) / PAGE_SIZE;
+
+ random_pages = get_random_long() % total_user_pages;
+ random_vaddr = FIRST_USER_ADDRESS + random_pages * PAGE_SIZE;
+
+ WARN_ON((random_vaddr > TASK_SIZE) ||
+ (random_vaddr < FIRST_USER_ADDRESS));
+ return random_vaddr;
+}
+
ramdom_vaddr is unsigned,
random_pages is unsigned and lower than total_user_pages
So the max value random_vaddr can get is FIRST_USER_ADDRESS + ((TASK_SIZE -
FIRST_USER_ADDRESS - 1) / PAGE_SIZE) * PAGE_SIZE = TASK_SIZE - 1
And the min value random_vaddr can get is FIRST_USER_ADDRESS (that's when
random_pages = 0)
That's right.
So the WARN_ON() is just unneeded, isn't it ?
It is just a sanity check on possible vaddr values before it's corresponding
page table mappings could be created. If it's worth to drop this in favor of
avoiding these unwanted warning messages on x86, will go ahead with it as it
is not super important.
But you are checking what ? That the compiler does calculation correctly
or what ?
As mentionned just above, based on the calculation done, what you are
testing cannot happen, so I'm having a hard time understanding what kind
of sanity check it can be.
Can you give an exemple of a situation which could trigger the warning ?
Christophe
Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
On 10/25/2019 12:41 PM, Christophe Leroy wrote:
>
>
> Le 25/10/2019 à 07:52, Qian Cai a écrit :
>>
>>
>>> On Oct 24, 2019, at 11:45 PM, Anshuman Khandual
>>> wrote:
>>>
>>> Nothing specific. But just tested this with x86 defconfig with relevant
>>> configs
>>> which are required for this test. Not sure if it involved W=1.
>>
>> No, it will not. It needs to run like,
>>
>> make W=1 -j 64 2>/tmp/warns
>>
>
> Are we talking about this peace of code ?
>
> +static unsigned long __init get_random_vaddr(void)
> +{
> + unsigned long random_vaddr, random_pages, total_user_pages;
> +
> + total_user_pages = (TASK_SIZE - FIRST_USER_ADDRESS) / PAGE_SIZE;
> +
> + random_pages = get_random_long() % total_user_pages;
> + random_vaddr = FIRST_USER_ADDRESS + random_pages * PAGE_SIZE;
> +
> + WARN_ON((random_vaddr > TASK_SIZE) ||
> + (random_vaddr < FIRST_USER_ADDRESS));
> + return random_vaddr;
> +}
> +
>
> ramdom_vaddr is unsigned,
> random_pages is unsigned and lower than total_user_pages
>
> So the max value random_vaddr can get is FIRST_USER_ADDRESS + ((TASK_SIZE -
> FIRST_USER_ADDRESS - 1) / PAGE_SIZE) * PAGE_SIZE = TASK_SIZE - 1
> And the min value random_vaddr can get is FIRST_USER_ADDRESS (that's when
> random_pages = 0)
That's right.
>
> So the WARN_ON() is just unneeded, isn't it ?
It is just a sanity check on possible vaddr values before it's corresponding
page table mappings could be created. If it's worth to drop this in favor of
avoiding these unwanted warning messages on x86, will go ahead with it as it
is not super important.
>
> Christophe
>
Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
Le 25/10/2019 à 07:52, Qian Cai a écrit :
On Oct 24, 2019, at 11:45 PM, Anshuman Khandual
wrote:
Nothing specific. But just tested this with x86 defconfig with relevant configs
which are required for this test. Not sure if it involved W=1.
No, it will not. It needs to run like,
make W=1 -j 64 2>/tmp/warns
Are we talking about this peace of code ?
+static unsigned long __init get_random_vaddr(void)
+{
+ unsigned long random_vaddr, random_pages, total_user_pages;
+
+ total_user_pages = (TASK_SIZE - FIRST_USER_ADDRESS) / PAGE_SIZE;
+
+ random_pages = get_random_long() % total_user_pages;
+ random_vaddr = FIRST_USER_ADDRESS + random_pages * PAGE_SIZE;
+
+ WARN_ON((random_vaddr > TASK_SIZE) ||
+ (random_vaddr < FIRST_USER_ADDRESS));
+ return random_vaddr;
+}
+
ramdom_vaddr is unsigned,
random_pages is unsigned and lower than total_user_pages
So the max value random_vaddr can get is FIRST_USER_ADDRESS +
((TASK_SIZE - FIRST_USER_ADDRESS - 1) / PAGE_SIZE) * PAGE_SIZE =
TASK_SIZE - 1
And the min value random_vaddr can get is FIRST_USER_ADDRESS (that's
when random_pages = 0)
So the WARN_ON() is just unneeded, isn't it ?
Christophe
Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
On 10/25/2019 11:22 AM, Qian Cai wrote: > > >> On Oct 24, 2019, at 11:45 PM, Anshuman Khandual >> wrote: >> >> Nothing specific. But just tested this with x86 defconfig with relevant >> configs >> which are required for this test. Not sure if it involved W=1. > > No, it will not. It needs to run like, > > make W=1 -j 64 2>/tmp/warns Ahh, so we explicitly ask for it. Unfortunately compiler still flags it as an warning. Just wondering why this is still a problem if the second condition for an OR expression is always false. Because evaluation still needs to be performed for the first condition anyways, before arriving at the result. DESCEND objtool CALLscripts/atomic/check-atomics.sh CALLscripts/checksyscalls.sh CHK include/generated/compile.h CC mm/debug_vm_pgtable.o In file included from ./arch/x86/include/asm/bug.h:83:0, from ./include/linux/bug.h:5, from ./include/linux/mmdebug.h:5, from ./include/linux/gfp.h:5, from mm/debug_vm_pgtable.c:13: mm/debug_vm_pgtable.c: In function ‘get_random_vaddr’: mm/debug_vm_pgtable.c:314:17: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] (random_vaddr < FIRST_USER_ADDRESS)); ^ ./include/asm-generic/bug.h:113:25: note: in definition of macro ‘WARN_ON’ int __ret_warn_on = !!(condition);\ ^ As you mentioned GCC is quite stubborn here. Anyways, lets keep it unchanged.
Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
> On Oct 24, 2019, at 11:45 PM, Anshuman Khandual > wrote: > > Nothing specific. But just tested this with x86 defconfig with relevant > configs > which are required for this test. Not sure if it involved W=1. No, it will not. It needs to run like, make W=1 -j 64 2>/tmp/warns
Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
On 10/24/2019 10:21 PM, Qian Cai wrote: > > >> On Oct 24, 2019, at 10:50 AM, Anshuman Khandual >> wrote: >> >> Changes in V7: >> >> - Memory allocation and free routines for mapped pages have been droped >> - Mapped pfns are derived from standard kernel text symbol per Matthew >> - Moved debug_vm_pgtaable() after page_alloc_init_late() per Michal and Qian >> - Updated the commit message per Michal >> - Updated W=1 GCC warning problem on x86 per Qian Cai > > It would be interesting to know if you actually tested out to see if the > warning went away. As far I can tell, the GCC is quite stubborn there, so I > am not going to insist. > Nothing specific. But just tested this with x86 defconfig with relevant configs which are required for this test. Not sure if it involved W=1. The problem is, there is no other or better way to have both the conditional checks in place while also reducing the chances this warning. IMHO both the conditional checks are required.
Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
> On Oct 24, 2019, at 10:50 AM, Anshuman Khandual > wrote: > > Changes in V7: > > - Memory allocation and free routines for mapped pages have been droped > - Mapped pfns are derived from standard kernel text symbol per Matthew > - Moved debug_vm_pgtaable() after page_alloc_init_late() per Michal and Qian > - Updated the commit message per Michal > - Updated W=1 GCC warning problem on x86 per Qian Cai It would be interesting to know if you actually tested out to see if the warning went away. As far I can tell, the GCC is quite stubborn there, so I am not going to insist.
Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
On 10/22/2019 12:41 PM, Christophe Leroy wrote: > > > On 10/21/2019 02:42 AM, Anshuman Khandual wrote: >> This adds tests which will validate architecture page table helpers and >> other accessors in their compliance with expected generic MM semantics. >> This will help various architectures in validating changes to existing >> page table helpers or addition of new ones. >> >> This test covers basic page table entry transformations including but not >> limited to old, young, dirty, clean, write, write protect etc at various >> level along with populating intermediate entries with next page table page >> and validating them. >> >> Test page table pages are allocated from system memory with required size >> and alignments. The mapped pfns at page table levels are derived from a >> real pfn representing a valid kernel text symbol. This test gets called >> right after page_alloc_init_late(). >> >> This gets build and run when CONFIG_DEBUG_VM_PGTABLE is selected along with >> CONFIG_VM_DEBUG. Architectures willing to subscribe this test also need to >> select CONFIG_ARCH_HAS_DEBUG_VM_PGTABLE which for now is limited to x86 and >> arm64. Going forward, other architectures too can enable this after fixing >> build or runtime problems (if any) with their page table helpers. >> >> Folks interested in making sure that a given platform's page table helpers >> conform to expected generic MM semantics should enable the above config >> which will just trigger this test during boot. Any non conformity here will >> be reported as an warning which would need to be fixed. This test will help >> catch any changes to the agreed upon semantics expected from generic MM and >> enable platforms to accommodate it thereafter. >> >> Cc: Andrew Morton >> Cc: Vlastimil Babka >> Cc: Greg Kroah-Hartman >> Cc: Thomas Gleixner >> Cc: Mike Rapoport >> Cc: Jason Gunthorpe >> Cc: Dan Williams >> Cc: Peter Zijlstra >> Cc: Michal Hocko >> Cc: Mark Rutland >> Cc: Mark Brown >> Cc: Steven Price >> Cc: Ard Biesheuvel >> Cc: Masahiro Yamada >> Cc: Kees Cook >> Cc: Tetsuo Handa >> Cc: Matthew Wilcox >> Cc: Sri Krishna chowdary >> Cc: Dave Hansen >> Cc: Russell King - ARM Linux >> Cc: Michael Ellerman >> Cc: Paul Mackerras >> Cc: Martin Schwidefsky >> Cc: Heiko Carstens >> Cc: "David S. Miller" >> Cc: Vineet Gupta >> Cc: James Hogan >> Cc: Paul Burton >> Cc: Ralf Baechle >> Cc: Kirill A. Shutemov >> Cc: Gerald Schaefer >> Cc: Christophe Leroy >> Cc: Ingo Molnar >> Cc: linux-snps-...@lists.infradead.org >> Cc: linux-m...@vger.kernel.org >> Cc: linux-arm-ker...@lists.infradead.org >> Cc: linux-i...@vger.kernel.org >> Cc: linuxppc-dev@lists.ozlabs.org >> Cc: linux-s...@vger.kernel.org >> Cc: linux...@vger.kernel.org >> Cc: sparcli...@vger.kernel.org >> Cc: x...@kernel.org >> Cc: linux-ker...@vger.kernel.org >> >> Tested-by: Christophe Leroy #PPC32 >> Suggested-by: Catalin Marinas >> Signed-off-by: Andrew Morton >> Signed-off-by: Christophe Leroy >> Signed-off-by: Anshuman Khandual >> --- > > The cover letter have the exact same title as this patch. I think a cover > letter is not necessary for a singleton series. Right, but it became singleton series in this version :) > > The history (and any other information you don't want to include in the > commit message) can be added here, below the '---'. That way it is in the > mail but won't be included in the commit. I was aware about that but the change log here was big, hence just choose to have that separately in a cover letter. As you said, I guess the cover letter is probably not required anymore. Will add it here in the patch, next time around. > >> .../debug/debug-vm-pgtable/arch-support.txt | 34 ++ >> arch/arm64/Kconfig | 1 + >> arch/x86/Kconfig | 1 + >> arch/x86/include/asm/pgtable_64.h | 6 + >> include/asm-generic/pgtable.h | 6 + >> init/main.c | 1 + >> lib/Kconfig.debug | 21 ++ >> mm/Makefile | 1 + >> mm/debug_vm_pgtable.c | 388 >> + >> 9 files changed, 459 insertions(+) >> create mode 100644 >> Documentation/features/debug/debug-vm-pgtable/arch-support.txt >> create mode 100644 mm/debug_vm_pgtable.c >> >> diff --git a/Documentation/features/debug/debug-vm-pgtable/arch-support.txt >> b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt >> new file mode 100644 >> index 000..d6b8185 >> --- /dev/null >> +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt >> @@ -0,0 +1,34 @@ >> +# >> +# Feature name: debug-vm-pgtable >> +# Kconfig: ARCH_HAS_DEBUG_VM_PGTABLE >> +# description: arch supports pgtable tests for semantics >> compliance >> +# >> + --- >>
Re: [PATCH V7] mm/debug: Add tests validating architecture page table helpers
On 10/21/2019 02:42 AM, Anshuman Khandual wrote: This adds tests which will validate architecture page table helpers and other accessors in their compliance with expected generic MM semantics. This will help various architectures in validating changes to existing page table helpers or addition of new ones. This test covers basic page table entry transformations including but not limited to old, young, dirty, clean, write, write protect etc at various level along with populating intermediate entries with next page table page and validating them. Test page table pages are allocated from system memory with required size and alignments. The mapped pfns at page table levels are derived from a real pfn representing a valid kernel text symbol. This test gets called right after page_alloc_init_late(). This gets build and run when CONFIG_DEBUG_VM_PGTABLE is selected along with CONFIG_VM_DEBUG. Architectures willing to subscribe this test also need to select CONFIG_ARCH_HAS_DEBUG_VM_PGTABLE which for now is limited to x86 and arm64. Going forward, other architectures too can enable this after fixing build or runtime problems (if any) with their page table helpers. Folks interested in making sure that a given platform's page table helpers conform to expected generic MM semantics should enable the above config which will just trigger this test during boot. Any non conformity here will be reported as an warning which would need to be fixed. This test will help catch any changes to the agreed upon semantics expected from generic MM and enable platforms to accommodate it thereafter. Cc: Andrew Morton Cc: Vlastimil Babka Cc: Greg Kroah-Hartman Cc: Thomas Gleixner Cc: Mike Rapoport Cc: Jason Gunthorpe Cc: Dan Williams Cc: Peter Zijlstra Cc: Michal Hocko Cc: Mark Rutland Cc: Mark Brown Cc: Steven Price Cc: Ard Biesheuvel Cc: Masahiro Yamada Cc: Kees Cook Cc: Tetsuo Handa Cc: Matthew Wilcox Cc: Sri Krishna chowdary Cc: Dave Hansen Cc: Russell King - ARM Linux Cc: Michael Ellerman Cc: Paul Mackerras Cc: Martin Schwidefsky Cc: Heiko Carstens Cc: "David S. Miller" Cc: Vineet Gupta Cc: James Hogan Cc: Paul Burton Cc: Ralf Baechle Cc: Kirill A. Shutemov Cc: Gerald Schaefer Cc: Christophe Leroy Cc: Ingo Molnar Cc: linux-snps-...@lists.infradead.org Cc: linux-m...@vger.kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-i...@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-s...@vger.kernel.org Cc: linux...@vger.kernel.org Cc: sparcli...@vger.kernel.org Cc: x...@kernel.org Cc: linux-ker...@vger.kernel.org Tested-by: Christophe Leroy #PPC32 Suggested-by: Catalin Marinas Signed-off-by: Andrew Morton Signed-off-by: Christophe Leroy Signed-off-by: Anshuman Khandual --- The cover letter have the exact same title as this patch. I think a cover letter is not necessary for a singleton series. The history (and any other information you don't want to include in the commit message) can be added here, below the '---'. That way it is in the mail but won't be included in the commit. .../debug/debug-vm-pgtable/arch-support.txt| 34 ++ arch/arm64/Kconfig | 1 + arch/x86/Kconfig | 1 + arch/x86/include/asm/pgtable_64.h | 6 + include/asm-generic/pgtable.h | 6 + init/main.c| 1 + lib/Kconfig.debug | 21 ++ mm/Makefile| 1 + mm/debug_vm_pgtable.c | 388 + 9 files changed, 459 insertions(+) create mode 100644 Documentation/features/debug/debug-vm-pgtable/arch-support.txt create mode 100644 mm/debug_vm_pgtable.c diff --git a/Documentation/features/debug/debug-vm-pgtable/arch-support.txt b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt new file mode 100644 index 000..d6b8185 --- /dev/null +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt @@ -0,0 +1,34 @@ +# +# Feature name: debug-vm-pgtable +# Kconfig: ARCH_HAS_DEBUG_VM_PGTABLE +# description: arch supports pgtable tests for semantics compliance +# +--- +| arch |status| +--- +| alpha: | TODO | +| arc: | TODO | +| arm: | TODO | +| arm64: | ok | +| c6x: | TODO | +|csky: | TODO | +| h8300: | TODO | +| hexagon: | TODO | +|ia64: | TODO | +|m68k: | TODO | +| microblaze: | TODO | +|mips: | TODO | +| nds32: | TODO | +| nios2: | TODO | +|openrisc: | TODO | +| parisc: | TODO | +| powerpc: | TODO | Say ok on ppc32 +| riscv: | TODO | +|s390: | TODO | +| sh: | TODO | +| sparc:
