Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 02/12/2020 11:25 PM, Gerald Schaefer wrote: > On Wed, 12 Feb 2020 15:12:54 +0530 > Anshuman Khandual wrote: > +/* + * On s390 platform, the lower 12 bits are used to identify given page table + * entry type and for other arch specific requirements. But these bits might + * affect the ability to clear entries with pxx_clear(). So while loading up + * the entries skip all lower 12 bits in order to accommodate s390 platform. + * It does not have affect any other platform. + */ +#define RANDOM_ORVALUE(0xf000UL) >>> >>> I'd suggest you generate this mask with something like >>> GENMASK(BITS_PER_LONG, PAGE_SHIFT). >> >> IIRC the lower 12 bits constrains on s390 platform might not be really >> related >> to it's PAGE_SHIFT which can be a variable, but instead just a constant >> number. >> But can definitely use GENMASK or it's variants here. >> >> https://lkml.org/lkml/2019/9/5/862 > > PAGE_SHIFT would be fine, it is 12 on s390. However, in order to be > more precise, we do not really need all 12 bits, only the last 4 bits. > So, something like this would work: > > #define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, 4) > > The text in the comment could then also be changed from 12 to 4, and > be a bit more specific on the fact that the impact on pxx_clear() > results from the dynamic page table folding logic on s390: > > /* > * On s390 platform, the lower 4 bits are used to identify given page table > * entry type. But these bits might affect the ability to clear entries with > * pxx_clear() because of how dynamic page table folding works on s390. So > * while loading up the entries do not change the lower 4 bits. > * It does not have affect any other platform. > */ Sure, will update accordingly.
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On Wed, 12 Feb 2020 15:12:54 +0530 Anshuman Khandual wrote: > >> +/* > >> + * On s390 platform, the lower 12 bits are used to identify given page > >> table > >> + * entry type and for other arch specific requirements. But these bits > >> might > >> + * affect the ability to clear entries with pxx_clear(). So while loading > >> up > >> + * the entries skip all lower 12 bits in order to accommodate s390 > >> platform. > >> + * It does not have affect any other platform. > >> + */ > >> +#define RANDOM_ORVALUE(0xf000UL) > > > > I'd suggest you generate this mask with something like > > GENMASK(BITS_PER_LONG, PAGE_SHIFT). > > IIRC the lower 12 bits constrains on s390 platform might not be really related > to it's PAGE_SHIFT which can be a variable, but instead just a constant > number. > But can definitely use GENMASK or it's variants here. > > https://lkml.org/lkml/2019/9/5/862 PAGE_SHIFT would be fine, it is 12 on s390. However, in order to be more precise, we do not really need all 12 bits, only the last 4 bits. So, something like this would work: #define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, 4) The text in the comment could then also be changed from 12 to 4, and be a bit more specific on the fact that the impact on pxx_clear() results from the dynamic page table folding logic on s390: /* * On s390 platform, the lower 4 bits are used to identify given page table * entry type. But these bits might affect the ability to clear entries with * pxx_clear() because of how dynamic page table folding works on s390. So * while loading up the entries do not change the lower 4 bits. * It does not have affect any other platform. */
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 02/10/2020 09:07 PM, Catalin Marinas wrote:
> On Tue, Jan 28, 2020 at 06:57:53AM +0530, Anshuman Khandual wrote:
>> 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.
>
> It may be worth posting the next version to linux-arch to reach out to
> other arch maintainers.
Sure, will do.
>
> Also I've seen that you posted a v13 but it hasn't reached
> linux-arm-kernel (likely held in moderation because of the large amount
> of addresses cc'ed) and I don't normally follow LKML. I'm not cc'ed to
> this patch either (which is fine as long as you post to a list that I
> read).
Right, the CC list on V13 was a disaster. I did not realize that it will
exceed the permitted limit when the lists will start refusing to take. In
fact, it looks like LKML did not get the email either.
>
> Since I started the reply on v12 about a week ago, I'll follow up here.
> When you post a v14, please trim the people on cc only to those strictly
> necessary (e.g. arch maintainers, linux-mm, linux-arch and lkml).
Sure, will do.
>
>> 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 ..f3f8111edbe3
>> --- /dev/null
>> +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt
>> @@ -0,0 +1,35 @@
>> +#
>> +# Feature name: debug-vm-pgtable
>> +# Kconfig: ARCH_HAS_DEBUG_VM_PGTABLE
>> +# description: arch supports pgtable tests for semantics
>> compliance
>> +#
>> +---
>> +| arch |status|
>> +---
>> +| alpha: | TODO |
>> +| arc: | ok |
>> +| arm: | TODO |
>
> I'm sure you can find some arm32 hardware around (or a VM) to give this
> a try ;).
It does not build on arm32 and we dont have an agreement on how to go about
that either, hence will disable this test on IA64 and ARM (32) in order to
prevent the known build failures (as Andrew had requested).
>
>> diff --git a/arch/x86/include/asm/pgtable_64.h
>> b/arch/x86/include/asm/pgtable_64.h
>> index 0b6c4042942a..fb0e76d254b3 100644
>> --- a/arch/x86/include/asm/pgtable_64.h
>> +++ b/arch/x86/include/asm/pgtable_64.h
> [...]
>> @@ -1197,6 +1197,7 @@ static noinline void __init kernel_init_freeable(void)
>> sched_init_smp();
>>
>> page_alloc_init_late();
>> +debug_vm_pgtable();
>> /* Initialize page ext after all struct pages are initialized. */
>> page_ext_init();
>
> I guess you could even make debug_vm_pgtable() an early_initcall(). I
> don't have a strong opinion either way.
>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> new file mode 100644
>> index ..0f37f32d15f1
>> --- /dev/null
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -0,0 +1,388 @@
> [...]
>> +/*
>> + * Basic operations
>> + *
>> + * mkold(entry) = An old and not a young entry
>> + * mkyoung(entry) = A young and not an old entry
>> + * mkdirty(entry) = A dirty and not a clean entry
>> + * mkclean(entry) = A clean and not a dirty entry
>> + * mkwrite(entry) = A write and not a write protected entry
>> + * wrprotect(entry) = A write protected and not a write entry
>> + * pxx_bad(entry) = A mapped and non-table entry
>> + * pxx_same(entry1, entry2) = Both entries hold the exact same value
>> + */
>> +#define VMFLAGS (VM_READ|VM_WRITE|VM_EXEC)
>> +
>> +/*
>> + * On s390 platform, the lower 12 bits are used to identify given page table
>> + * entry type and for other arch specific requirements. But these bits might
>> + * affect the ability to clear entries with pxx_clear(). So while loading up
>> + * the entries skip all lower 12 bits in order to accommodate s390 platform.
>> + * It does not have affect any other platform.
>> + */
>> +#define RANDOM_ORVALUE (0xf000UL)
>
> I'd suggest you generate this mask with something like
> GENMASK(BITS_PER_LONG, PAGE_SHIFT).
IIRC the lower 12 bits constrains on s390 platform might not be really related
to it's PAGE_SHIFT which can be a variable, but instead just a constant number.
But can definitely use GENMASK or it's variants here.
https://lkml.org/lkml/2019/9/5/862
>
>> +#define RANDOM_NZVALUE (0xff)
>> +
>> +static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
>> +{
>> +pte_t pte = pfn_pte(pfn, prot);
>> +
>> +WARN_ON(!pte_same(pte, pte));
>> +WARN_ON(!pte_young(pte_mkyoung(pte)));
>> +WARN_ON(!pte_dirty(pte_mkdirty(pte)));
>> +WARN_ON(!pte_write(pte_mkwrite(pte)));
>> +
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On Tue, Jan 28, 2020 at 06:57:53AM +0530, Anshuman Khandual wrote:
> 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.
It may be worth posting the next version to linux-arch to reach out to
other arch maintainers.
Also I've seen that you posted a v13 but it hasn't reached
linux-arm-kernel (likely held in moderation because of the large amount
of addresses cc'ed) and I don't normally follow LKML. I'm not cc'ed to
this patch either (which is fine as long as you post to a list that I
read).
Since I started the reply on v12 about a week ago, I'll follow up here.
When you post a v14, please trim the people on cc only to those strictly
necessary (e.g. arch maintainers, linux-mm, linux-arch and lkml).
> 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 ..f3f8111edbe3
> --- /dev/null
> +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt
> @@ -0,0 +1,35 @@
> +#
> +# Feature name: debug-vm-pgtable
> +# Kconfig: ARCH_HAS_DEBUG_VM_PGTABLE
> +# description: arch supports pgtable tests for semantics compliance
> +#
> +---
> +| arch |status|
> +---
> +| alpha: | TODO |
> +| arc: | ok |
> +| arm: | TODO |
I'm sure you can find some arm32 hardware around (or a VM) to give this
a try ;).
> diff --git a/arch/x86/include/asm/pgtable_64.h
> b/arch/x86/include/asm/pgtable_64.h
> index 0b6c4042942a..fb0e76d254b3 100644
> --- a/arch/x86/include/asm/pgtable_64.h
> +++ b/arch/x86/include/asm/pgtable_64.h
[...]
> @@ -1197,6 +1197,7 @@ static noinline void __init kernel_init_freeable(void)
> sched_init_smp();
>
> page_alloc_init_late();
> + debug_vm_pgtable();
> /* Initialize page ext after all struct pages are initialized. */
> page_ext_init();
I guess you could even make debug_vm_pgtable() an early_initcall(). I
don't have a strong opinion either way.
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> new file mode 100644
> index ..0f37f32d15f1
> --- /dev/null
> +++ b/mm/debug_vm_pgtable.c
> @@ -0,0 +1,388 @@
[...]
> +/*
> + * Basic operations
> + *
> + * mkold(entry) = An old and not a young entry
> + * mkyoung(entry)= A young and not an old entry
> + * mkdirty(entry)= A dirty and not a clean entry
> + * mkclean(entry)= A clean and not a dirty entry
> + * mkwrite(entry)= A write and not a write protected entry
> + * wrprotect(entry) = A write protected and not a write entry
> + * pxx_bad(entry)= A mapped and non-table entry
> + * pxx_same(entry1, entry2) = Both entries hold the exact same value
> + */
> +#define VMFLAGS (VM_READ|VM_WRITE|VM_EXEC)
> +
> +/*
> + * On s390 platform, the lower 12 bits are used to identify given page table
> + * entry type and for other arch specific requirements. But these bits might
> + * affect the ability to clear entries with pxx_clear(). So while loading up
> + * the entries skip all lower 12 bits in order to accommodate s390 platform.
> + * It does not have affect any other platform.
> + */
> +#define RANDOM_ORVALUE (0xf000UL)
I'd suggest you generate this mask with something like
GENMASK(BITS_PER_LONG, PAGE_SHIFT).
> +#define RANDOM_NZVALUE (0xff)
> +
> +static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pte_t pte = pfn_pte(pfn, prot);
> +
> + WARN_ON(!pte_same(pte, pte));
> + WARN_ON(!pte_young(pte_mkyoung(pte)));
> + WARN_ON(!pte_dirty(pte_mkdirty(pte)));
> + WARN_ON(!pte_write(pte_mkwrite(pte)));
> + WARN_ON(pte_young(pte_mkold(pte)));
> + WARN_ON(pte_dirty(pte_mkclean(pte)));
> + WARN_ON(pte_write(pte_wrprotect(pte)));
Given that you start with rwx permissions set,
some of these ops would not have any effect. For example, on arm64 at
least, mkwrite clears a bit already cleared here. You could try with
multiple rwx combinations values (e.g. all set and all cleared) or maybe
something like below:
WARN_ON(!pte_write(pte_mkwrite(pte_wrprotect(pte;
You could also try something like this:
WARN_ON(!pte_same(pte_wrprotect(pte), pte_wrprotect(pte_mkwrite(pte;
though the above approach may not work for arm64 ptep_set_wrprotect() on
a dirty pte (if you extend these tests later).
> +}
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static void __init pmd_basic_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pmd_t pmd = pfn_pmd(pfn,
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On Mon, 2020-02-03 at 16:14 +0100, Christophe Leroy wrote: > > Le 02/02/2020 à 12:26, Qian Cai a écrit : > > > > > > > On Jan 30, 2020, at 9:13 AM, Christophe Leroy > > > wrote: > > > > > > config DEBUG_VM_PGTABLE > > > bool "Debug arch page table for semantics compliance" if > > > ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT > > > depends on MMU > > > default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE > > > default 'y' if DEBUG_VM > > > > Does it really necessary to potentially force all bots to run this? Syzbot, > > kernel test robot etc? Does it ever pay off for all their machine times > > there? > > > > Machine time ? > > On a 32 bits powerpc running at 132 MHz, the tests takes less than 10ms. > Is it worth taking the risk of not detecting faults by not selecting it > by default ? The risk is quite low as Catalin mentioned this thing is not to detect regressions but rather for arch/mm maintainers. I do appreciate the efforts to get everyone as possible to run this thing, so it get more notices once it is broken. However, DEBUG_VM seems like such a generic Kconfig those days that have even been enabled by default for Fedora Linux, so I would rather see a more sensitive default been taken even though the test runtime is fairly quickly on a small machine for now. > > [5.656916] debug_vm_pgtable: debug_vm_pgtable: Validating > architecture page table helpers > [5.665661] debug_vm_pgtable: debug_vm_pgtable: Validated > architecture page table helpers
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
Le 02/02/2020 à 12:26, Qian Cai a écrit : On Jan 30, 2020, at 9:13 AM, Christophe Leroy wrote: config DEBUG_VM_PGTABLE bool "Debug arch page table for semantics compliance" if ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT depends on MMU default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE default 'y' if DEBUG_VM Does it really necessary to potentially force all bots to run this? Syzbot, kernel test robot etc? Does it ever pay off for all their machine times there? Machine time ? On a 32 bits powerpc running at 132 MHz, the tests takes less than 10ms. Is it worth taking the risk of not detecting faults by not selecting it by default ? [5.656916] debug_vm_pgtable: debug_vm_pgtable: Validating architecture page table helpers [5.665661] debug_vm_pgtable: debug_vm_pgtable: Validated architecture page table helpers Christophe
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 30, 2020, at 9:13 AM, Christophe Leroy wrote: > > config DEBUG_VM_PGTABLE >bool "Debug arch page table for semantics compliance" if > ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT >depends on MMU >default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE >default 'y' if DEBUG_VM Does it really necessary to potentially force all bots to run this? Syzbot, kernel test robot etc? Does it ever pay off for all their machine times there?
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
Le 02/02/2020 à 08:18, Anshuman Khandual a écrit : On 01/30/2020 07:43 PM, Christophe Leroy wrote: Le 30/01/2020 à 14:04, Anshuman Khandual a écrit : On 01/28/2020 10:35 PM, Christophe Leroy wrote: I think we could make it standalone and 'default y if DEBUG_VM' instead. Which will yield the same result like before but in a different way. But yes, this test could go about either way but unless there is a good enough reason why change the current one. I think if we want people to really use it on other architectures it must be possible to activate it without having to modify Kconfig. Otherwise people won't even know the test exists and the architecture fails the test. The purpose of a test suite is to detect bugs. If you can't run the test until you have fixed the bugs, I guess nobody will ever detect the bugs and they will never be fixed. So I think: - the test should be 'default y' when ARCH_HAS_DEBUG_VM_PGTABLE is selected - the test should be 'default n' when ARCH_HAS_DEBUG_VM_PGTABLE is not selected, and it should be user selectable if EXPERT is selected. Something like: config DEBUG_VM_PGTABLE bool "Debug arch page table for semantics compliance" if ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT depends on MMU (ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT) be moved along side MMU on the same line ? Yes could also go along side MMU, or could be a depend by itself: depends on ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE default 'y' if DEBUG_VM This looks good, at least until we get all platforms enabled. Will do all these changes along with s390 enablement and re-spin. Christophe
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 01/30/2020 06:34 PM, Anshuman Khandual wrote: > On 01/28/2020 10:35 PM, Christophe Leroy wrote: >> >> Le 28/01/2020 à 02:27, Anshuman Khandual a écrit : >>> 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. >>> >> [...] >> >>> Tested-by: Christophe Leroy #PPC32 >> Also tested on PPC64 (under QEMU): book3s/64 64k pages, book3s/64 4k pages >> and book3e/64 > Hmm but earlier Michael Ellerman had reported some problems while > running these tests on PPC64, a soft lock up in hash__pte_update() > and a kernel BUG (radix MMU). Are those problems gone away now ? > > Details in this thread - https://patchwork.kernel.org/patch/11214603/ > It is always better to have more platforms enabled than not. But lets keep this test disabled on PPC64 for now, if there is any inconsistency between results while running this under QEMU and on actual systems.
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 01/30/2020 07:43 PM, Christophe Leroy wrote:
>
>
> Le 30/01/2020 à 14:04, Anshuman Khandual a écrit :
>>
>> On 01/28/2020 10:35 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 28/01/2020 à 02:27, Anshuman Khandual a écrit :
diff --git a/arch/x86/include/asm/pgtable_64.h
b/arch/x86/include/asm/pgtable_64.h
index 0b6c4042942a..fb0e76d254b3 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -53,6 +53,12 @@ static inline void sync_initial_page_table(void) { }
struct mm_struct;
+#define mm_p4d_folded mm_p4d_folded
+static inline bool mm_p4d_folded(struct mm_struct *mm)
+{
+ return !pgtable_l5_enabled();
+}
+
>>>
>>> For me this should be part of another patch, it is not directly linked to
>>> the tests.
>>
>> We did discuss about this earlier and Kirril mentioned its not worth
>> a separate patch.
>>
>> https://lore.kernel.org/linux-arm-kernel/20190913091305.rkds4f3fqv3yjhjy@box/
>
> For me it would make sense to not mix this patch which implement tests, and
> changes that are needed for the test to work (or even build) on the different
> architectures.
>
> But that's up to you.
>
>>
>>>
void set_pte_vaddr_p4d(p4d_t *p4d_page, unsigned long vaddr, pte_t
new_pte);
void set_pte_vaddr_pud(pud_t *pud_page, unsigned long vaddr, pte_t
new_pte);
diff --git a/include/asm-generic/pgtable.h
b/include/asm-generic/pgtable.h
index 798ea36a0549..e0b04787e789 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -1208,6 +1208,12 @@ static inline bool arch_has_pfn_modify_check(void)
# define PAGE_KERNEL_EXEC PAGE_KERNEL
#endif
+#ifdef CONFIG_DEBUG_VM_PGTABLE
>>>
>>> Not sure it is a good idea to put that in include/asm-generic/pgtable.h
>>
>> Logically that is the right place, as it is related to page table but
>> not something platform related.
>
> I can't see any debug related features in that file.
>
>>
>>>
>>> By doing this you are forcing a rebuild of almost all files, whereas only
>>> init/main.o and mm/debug_vm_pgtable.o should be rebuilt when activating
>>> this config option.
>>
>> I agreed but whats the alternative ? We could move these into init/main.c
>> to make things simpler but will that be a right place, given its related
>> to generic page table.
>
> What about linux/mmdebug.h instead ? (I have not checked if it would reduce
> the impact, but that's where things related to CONFIG_DEBUG_VM seems to be).
>
> Otherwise, you can just create new file, for instance
> and include that file only in the init/main.c and
> mm/debug_vm_pgtable.c
IMHO it might not be wise to add yet another header file for this purpose.
Instead lets use in line with DEBUG_VM, DEBUG_VM_PGFLAGS,
DEBUG_VIRTUAL (which is also a stand alone test). A simple grep shows that
the impact of mmdebug.h would be less than generic pgtable.h header.
>
>
>
>>
>>>
+extern void debug_vm_pgtable(void);
>>>
>>> Please don't use the 'extern' keyword, it is useless and not to be used for
>>> functions declaration.
>>
>> Really ? But, there are tons of examples doing the same thing both in
>> generic and platform code as well.
>
> Yes, but how can we improve if we blindly copy the errors from the past ?
> Having tons of 'extern' doesn't mean we must add more.
>
> I think checkpatch.pl usually complains when a patch brings a new unreleval
> extern symbol.
Sure np, will drop it. But checkpatch.pl never complained.
>
>>
>>>
+#else
+static inline void debug_vm_pgtable(void) { }
+#endif
+
#endif /* !__ASSEMBLY__ */
#ifndef io_remap_pfn_range
diff --git a/init/main.c b/init/main.c
index da1bc0b60a7d..5e59e6ac0780 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1197,6 +1197,7 @@ static noinline void __init
kernel_init_freeable(void)
sched_init_smp();
page_alloc_init_late();
+ debug_vm_pgtable();
>>>
>>> Wouldn't it be better to call debug_vm_pgtable() in kernel_init() between
>>> the call to async_synchronise_full() and ftrace_free_init_mem() ?
>>
>> IIRC, proposed location is the earliest we could call debug_vm_pgtable().
>> Is there any particular benefit or reason to move it into kernel_init() ?
>
> It would avoid having it lost in the middle of drivers logs, would be close
> to the end of init, at a place we can't miss it, close to the result of other
> tests like CONFIG_DEBUG_RODATA_TEST for instance.
>
> At the moment, you have to look for it to be sure the test is done and what
> the result is.
Sure, will move it.
>
>>
>>>
/* Initialize page ext after all struct pages are initialized. */
page_ext_init();
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5ffe144c9794..7cceae923c05 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 01/28/2020 06:57 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 I should have included mailing lists for all missing platforms here. Will add them in the patch next time around but for now just adding them here explicitly so that hopefully in case some of them can build and run the test successfully on respective platforms. ALPHA: + linux-al...@vger.kernel.org + Richard Henderson + Ivan Kokshaysky + Matt Turner C6X: + linux-c6x-...@linux-c6x.org + Mark Salter + Aurelien Jacquiot H8300: + uclinux-h8-de...@lists.sourceforge.jp + Yoshinori Sato HEXAGON: + linux-hexa...@vger.kernel.org + Brian Cain M68K: + linux-m...@lists.linux-m68k.org + Geert Uytterhoeven MICROBLAZE: + Michal Simek RISCV: + linux-ri...@lists.infradead.org + Paul Walmsley + Palmer Dabbelt UNICORE32: + Guan Xuetao XTENSA: + linux-xte...@linux-xtensa.org + Chris Zankel + Max Filippov Please feel free to add others if I have missed.
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
Le 30/01/2020 à 14:04, Anshuman Khandual a écrit :
On 01/28/2020 10:35 PM, Christophe Leroy wrote:
Le 28/01/2020 à 02:27, Anshuman Khandual a écrit :
diff --git a/arch/x86/include/asm/pgtable_64.h
b/arch/x86/include/asm/pgtable_64.h
index 0b6c4042942a..fb0e76d254b3 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -53,6 +53,12 @@ static inline void sync_initial_page_table(void) { }
struct mm_struct;
+#define mm_p4d_folded mm_p4d_folded
+static inline bool mm_p4d_folded(struct mm_struct *mm)
+{
+ return !pgtable_l5_enabled();
+}
+
For me this should be part of another patch, it is not directly linked to the
tests.
We did discuss about this earlier and Kirril mentioned its not worth
a separate patch.
https://lore.kernel.org/linux-arm-kernel/20190913091305.rkds4f3fqv3yjhjy@box/
For me it would make sense to not mix this patch which implement tests,
and changes that are needed for the test to work (or even build) on the
different architectures.
But that's up to you.
void set_pte_vaddr_p4d(p4d_t *p4d_page, unsigned long vaddr, pte_t new_pte);
void set_pte_vaddr_pud(pud_t *pud_page, unsigned long vaddr, pte_t new_pte);
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index 798ea36a0549..e0b04787e789 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -1208,6 +1208,12 @@ static inline bool arch_has_pfn_modify_check(void)
# define PAGE_KERNEL_EXEC PAGE_KERNEL
#endif
+#ifdef CONFIG_DEBUG_VM_PGTABLE
Not sure it is a good idea to put that in include/asm-generic/pgtable.h
Logically that is the right place, as it is related to page table but
not something platform related.
I can't see any debug related features in that file.
By doing this you are forcing a rebuild of almost all files, whereas only
init/main.o and mm/debug_vm_pgtable.o should be rebuilt when activating this
config option.
I agreed but whats the alternative ? We could move these into init/main.c
to make things simpler but will that be a right place, given its related
to generic page table.
What about linux/mmdebug.h instead ? (I have not checked if it would
reduce the impact, but that's where things related to CONFIG_DEBUG_VM
seems to be).
Otherwise, you can just create new file, for instance
and include that file only in the init/main.c
and mm/debug_vm_pgtable.c
+extern void debug_vm_pgtable(void);
Please don't use the 'extern' keyword, it is useless and not to be used for
functions declaration.
Really ? But, there are tons of examples doing the same thing both in
generic and platform code as well.
Yes, but how can we improve if we blindly copy the errors from the past
? Having tons of 'extern' doesn't mean we must add more.
I think checkpatch.pl usually complains when a patch brings a new
unreleval extern symbol.
+#else
+static inline void debug_vm_pgtable(void) { }
+#endif
+
#endif /* !__ASSEMBLY__ */
#ifndef io_remap_pfn_range
diff --git a/init/main.c b/init/main.c
index da1bc0b60a7d..5e59e6ac0780 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1197,6 +1197,7 @@ static noinline void __init kernel_init_freeable(void)
sched_init_smp();
page_alloc_init_late();
+ debug_vm_pgtable();
Wouldn't it be better to call debug_vm_pgtable() in kernel_init() between the
call to async_synchronise_full() and ftrace_free_init_mem() ?
IIRC, proposed location is the earliest we could call debug_vm_pgtable().
Is there any particular benefit or reason to move it into kernel_init() ?
It would avoid having it lost in the middle of drivers logs, would be
close to the end of init, at a place we can't miss it, close to the
result of other tests like CONFIG_DEBUG_RODATA_TEST for instance.
At the moment, you have to look for it to be sure the test is done and
what the result is.
/* Initialize page ext after all struct pages are initialized. */
page_ext_init();
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5ffe144c9794..7cceae923c05 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -653,6 +653,12 @@ config SCHED_STACK_END_CHECK
data corruption or a sporadic crash at a later stage once the region
is examined. The runtime overhead introduced is minimal.
+config ARCH_HAS_DEBUG_VM_PGTABLE
+ bool
+ help
+ An architecture should select this when it can successfully
+ build and run DEBUG_VM_PGTABLE.
+
config DEBUG_VM
bool "Debug VM"
depends on DEBUG_KERNEL
@@ -688,6 +694,22 @@ config DEBUG_VM_PGFLAGS
If unsure, say N.
+config DEBUG_VM_PGTABLE
+ bool "Debug arch page table for semantics compliance"
+ depends on MMU
+ depends on DEBUG_VM
Does it really need to depend on DEBUG_VM ?
No. It seemed better to package this test along with DEBUG_VM (although I
dont remember the conversation around it) and hence this dependency.
Yes
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 01/30/2020 12:57 PM, Mike Rapoport wrote: > On Wed, Jan 29, 2020 at 11:20:44PM +0100, Gerald Schaefer wrote: >> On Mon, 27 Jan 2020 22:33:08 -0500 >> >> For example, who would have thought that pXd_bad() is supposed to >> report large entries as bad? It's not really documented anywhere, > > A bit off-topic, > > @Anshuman, maybe you could start a Documentation/ patch that describes at > least some of the pXd_whaterver()? > Or that would be too much to ask? ;-) No, it would not be :) I have been documenting the expected semantics for the helpers in the test itself. The idea is to collate them all (have been working on some additional tests but waiting for this one to get merged first) here and once most of the test gets settled, will move semantics documentation from here into Documentation/ directory in a proper format. /* * Basic operations * * mkold(entry) = An old and not a young entry * mkyoung(entry) = A young and not an old entry * mkdirty(entry) = A dirty and not a clean entry * mkclean(entry) = A clean and not a dirty entry * mkwrite(entry) = A write and not a write protected entry * wrprotect(entry) = A write protected and not a write entry * pxx_bad(entry) = A mapped and non-table entry * pxx_same(entry1, entry2) = Both entries hold the exact same value */ > >> so we just checked them for sanity like normal entries, which >> apparently worked fine so far, but for how long? >
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 01/30/2020 03:50 AM, Gerald Schaefer wrote: > On Tue, 28 Jan 2020 06:57:53 +0530 > 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. >> > > [...] > >> >> Tested-by: Christophe Leroy #PPC32 > > Tested-by: Gerald Schaefer # s390 Thanks for testing. > > Thanks again for this effort, and for keeping up the spirit against > all odds and even after 12 iterations :-) > >> >> 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 ..f3f8111edbe3 >> --- /dev/null >> +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt >> @@ -0,0 +1,35 @@ >> +# >> +# Feature name: debug-vm-pgtable >> +# Kconfig: ARCH_HAS_DEBUG_VM_PGTABLE >> +# description: arch supports pgtable tests for semantics >> compliance >> +# >> +--- >> +| arch |status| >> +--- >> +| alpha: | TODO | >> +| arc: | ok | >> +| 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/32: | ok | >> +| powerpc/64: | TODO | >> +| riscv: | TODO | >> +|s390: | TODO | > > s390 is ok now, with my patches included in v5.5-rc1. So you can now add > > --- a/Documentation/features/debug/debug-vm-pgtable/arch-support.txt > +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt > @@ -25,7 +25,7 @@ > | powerpc/32: | ok | > | powerpc/64: | TODO | > | riscv: | TODO | > -|s390: | TODO | > +|s390: | ok | > | sh: | TODO | > | sparc: | TODO | > | um: | TODO | > --- a/arch/s390/Kconfig > +++ b/arch/s390/Kconfig > @@ -59,6 +59,7 @@ config KASAN_SHADOW_OFFSET > config S390 > def_bool y > select ARCH_BINFMT_ELF_STATE > + select ARCH_HAS_DEBUG_VM_PGTABLE > select ARCH_HAS_DEVMEM_IS_ALLOWED > select ARCH_HAS_ELF_RANDOMIZE > select ARCH_HAS_FORTIFY_SOURCE Sure, will add this up.
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 01/28/2020 10:35 PM, Christophe Leroy wrote:
>
>
> Le 28/01/2020 à 02:27, Anshuman Khandual a écrit :
>> 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.
>>
>
> [...]
>
>>
>> Tested-by: Christophe Leroy #PPC32
>
> Also tested on PPC64 (under QEMU): book3s/64 64k pages, book3s/64 4k pages
> and book3e/64
Hmm but earlier Michael Ellerman had reported some problems while
running these tests on PPC64, a soft lock up in hash__pte_update()
and a kernel BUG (radix MMU). Are those problems gone away now ?
Details in this thread - https://patchwork.kernel.org/patch/11214603/
>
>> Reviewed-by: Ingo Molnar
>> Suggested-by: Catalin Marinas
>> Signed-off-by: Andrew Morton
>> Signed-off-by: Christophe Leroy
>> Signed-off-by: Anshuman Khandual
>> ---
>
> [...]
>
>>
>> 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 ..f3f8111edbe3
>> --- /dev/null
>> +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt
>> @@ -0,0 +1,35 @@
>> +#
>> +# Feature name: debug-vm-pgtable
>> +# Kconfig: ARCH_HAS_DEBUG_VM_PGTABLE
>> +# description: arch supports pgtable tests for semantics
>> compliance
>> +#
>> + ---
>> + | arch |status|
>> + ---
>> + | alpha: | TODO |
>> + | arc: | ok |
>> + | 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/32: | ok |
>> + | powerpc/64: | TODO |
>
> You can change the two above lines by
>
> powerpc: ok
>
>> + | riscv: | TODO |
>> + | s390: | TODO |
>> + | sh: | TODO |
>> + | sparc: | TODO |
>> + | um: | TODO |
>> + | unicore32: | TODO |
>> + | x86: | ok |
>> + | xtensa: | TODO |
>> + ---
>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 1ec34e16ed65..253dcab0bebc 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -120,6 +120,7 @@ config PPC
>> #
>> select ARCH_32BIT_OFF_T if PPC32
>> select ARCH_HAS_DEBUG_VIRTUAL
>> + select ARCH_HAS_DEBUG_VM_PGTABLE if PPC32
>
> Remove the 'if PPC32' as we now know it also work on PPC64.
But in case there is a subset of PPC64 which still does not work
(problem reported earlier) with the test, will have to adjust the
config accordingly.
>
>> select ARCH_HAS_DEVMEM_IS_ALLOWED
>> select ARCH_HAS_ELF_RANDOMIZE
>> select ARCH_HAS_FORTIFY_SOURCE
>
>> diff --git a/arch/x86/include/asm/pgtable_64.h
>> b/arch/x86/include/asm/pgtable_64.h
>> index 0b6c4042942a..fb0e76d254b3 100644
>> --- a/arch/x86/include/asm/pgtable_64.h
>> +++ b/arch/x86/include/asm/pgtable_64.h
>> @@ -53,6 +53,12 @@ static inline void sync_initial_page_table(void) { }
>> struct mm_struct;
>> +#define mm_p4d_folded mm_p4d_folded
>> +static inline bool
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On Wed, Jan 29, 2020 at 11:20:44PM +0100, Gerald Schaefer wrote: > On Mon, 27 Jan 2020 22:33:08 -0500 > > For example, who would have thought that pXd_bad() is supposed to > report large entries as bad? It's not really documented anywhere, A bit off-topic, @Anshuman, maybe you could start a Documentation/ patch that describes at least some of the pXd_whaterver()? Or that would be too much to ask? ;-) > so we just checked them for sanity like normal entries, which > apparently worked fine so far, but for how long? -- Sincerely yours, Mike.
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On Mon, 27 Jan 2020 22:33:08 -0500 Qian Cai wrote: > > > >> Did those tests ever find any regression or this is almost only useful for > >> new > > > > The test has already found problems with s390 page table helpers. > > Hmm, that is pretty weak where s390 is not even official supported with this > version. > I first had to get the three patches upstream, each fixing non-conform behavior on s390, and each issue was found by this extremely useful test: 2416cefc504b s390/mm: add mm_pxd_folded() checks to pxd_free() 2d1fc1eb9b54 s390/mm: simplify page table helpers for large entries 1c27a4bc817b s390/mm: make pmd/pud_bad() report large entries as bad I did not see any direct effect of this misbehavior yet, but I am very happy that this could be found and fixed in order to prevent future issues. And this is exactly the value of this test, to make sure that all architectures have a common understanding of how the various page table helpers are supposed to work. For example, who would have thought that pXd_bad() is supposed to report large entries as bad? It's not really documented anywhere, so we just checked them for sanity like normal entries, which apparently worked fine so far, but for how long?
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On Tue, 28 Jan 2020 06:57:53 +0530 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. > [...] > > Tested-by: Christophe Leroy #PPC32 Tested-by: Gerald Schaefer # s390 Thanks again for this effort, and for keeping up the spirit against all odds and even after 12 iterations :-) > > 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 ..f3f8111edbe3 > --- /dev/null > +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt > @@ -0,0 +1,35 @@ > +# > +# Feature name: debug-vm-pgtable > +# Kconfig: ARCH_HAS_DEBUG_VM_PGTABLE > +# description: arch supports pgtable tests for semantics compliance > +# > +--- > +| arch |status| > +--- > +| alpha: | TODO | > +| arc: | ok | > +| 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/32: | ok | > +| powerpc/64: | TODO | > +| riscv: | TODO | > +|s390: | TODO | s390 is ok now, with my patches included in v5.5-rc1. So you can now add --- a/Documentation/features/debug/debug-vm-pgtable/arch-support.txt +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt @@ -25,7 +25,7 @@ | powerpc/32: | ok | | powerpc/64: | TODO | | riscv: | TODO | -|s390: | TODO | +|s390: | ok | | sh: | TODO | | sparc: | TODO | | um: | TODO | --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -59,6 +59,7 @@ config KASAN_SHADOW_OFFSET config S390 def_bool y select ARCH_BINFMT_ELF_STATE + select ARCH_HAS_DEBUG_VM_PGTABLE select ARCH_HAS_DEVMEM_IS_ALLOWED select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_FORTIFY_SOURCE
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 29, 2020, at 5:36 AM, Catalin Marinas wrote: > > On Tue, Jan 28, 2020 at 02:07:10PM -0500, Qian Cai wrote: >> On Jan 28, 2020, at 12:47 PM, Catalin Marinas >> wrote: >>> The primary goal here is not finding regressions but having clearly >>> defined semantics of the page table accessors across architectures. x86 >>> and arm64 are a good starting point and other architectures will be >>> enabled as they are aligned to the same semantics. >> >> This still does not answer the fundamental question. If this test is >> simply inefficient to find bugs, > > Who said this is inefficient (other than you)? Inefficient of finding bugs. It said only found a bug or two in its lifetime? > >> who wants to spend time to use it regularly? > > Arch maintainers, mm maintainers introducing new macros or assuming > certain new semantics of the existing macros. > >> If this is just one off test that may get running once in a few years >> (when introducing a new arch), how does it justify the ongoing cost to >> maintain it? > > You are really missing the point. It's not only for a new arch but > changes to existing arch code. And if the arch code churn in this area > is relatively small, I'd expect a similarly small cost of maintaining > this test. > > If you only turn DEBUG_VM on once every few years, don't generalise this > to the rest of the kernel developers (as others pointed out, this test > is default y if DEBUG_VM). Quite the opposite, I am running DEBUG_VM almost daily for regression workload while I felt strongly this thing does not add any value mixing there. So, I would suggest to decouple this away from DEBUG_VM, and clearly document that this test is not something intended for automated regression workloads, so those people don’t need to waste time running this. > > Anyway, I think that's a pointless discussion, so not going to reply > further (unless you have technical content to add). > > -- > Catalin
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On Tue, Jan 28, 2020 at 02:07:10PM -0500, Qian Cai wrote: > On Jan 28, 2020, at 12:47 PM, Catalin Marinas wrote: > > The primary goal here is not finding regressions but having clearly > > defined semantics of the page table accessors across architectures. x86 > > and arm64 are a good starting point and other architectures will be > > enabled as they are aligned to the same semantics. > > This still does not answer the fundamental question. If this test is > simply inefficient to find bugs, Who said this is inefficient (other than you)? > who wants to spend time to use it regularly? Arch maintainers, mm maintainers introducing new macros or assuming certain new semantics of the existing macros. > If this is just one off test that may get running once in a few years > (when introducing a new arch), how does it justify the ongoing cost to > maintain it? You are really missing the point. It's not only for a new arch but changes to existing arch code. And if the arch code churn in this area is relatively small, I'd expect a similarly small cost of maintaining this test. If you only turn DEBUG_VM on once every few years, don't generalise this to the rest of the kernel developers (as others pointed out, this test is default y if DEBUG_VM). Anyway, I think that's a pointless discussion, so not going to reply further (unless you have technical content to add). -- Catalin
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 28, 2020, at 12:47 PM, Catalin Marinas wrote: > > The primary goal here is not finding regressions but having clearly > defined semantics of the page table accessors across architectures. x86 > and arm64 are a good starting point and other architectures will be > enabled as they are aligned to the same semantics. This still does not answer the fundamental question. If this test is simply inefficient to find bugs, who wants to spend time to use it regularly? If this is just one off test that may get running once in a few years (when introducing a new arch), how does it justify the ongoing cost to maintain it? I do agree there could be a need to clearly define this thing but that belongs to documentation rather than testing purpose. It is confusing to mix this with other config options which have somewhat a different purpose, it will then be a waste of time for people who mistakenly enable this for regular automatic testing and never found any bug from it.
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On Mon, Jan 27, 2020 at 09:11:53PM -0500, Qian Cai wrote: > On Jan 27, 2020, at 8:28 PM, 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. [...] > What’s the value of this block of new code? It only supports x86 and > arm64 which are supposed to be good now. Did those tests ever find any > regression or this is almost only useful for new architectures which > only happened once in a few years? The primary goal here is not finding regressions but having clearly defined semantics of the page table accessors across architectures. x86 and arm64 are a good starting point and other architectures will be enabled as they are aligned to the same semantics. See for example this past discussion: https://lore.kernel.org/linux-mm/20190628102003.ga56...@arrakis.emea.arm.com/ These tests should act as the 'contract' between the generic mm code and the architecture port. Without clear semantics, some bugs may be a lot subtler than a boot failure. FTR, I fully support this patch (and I should get around to review it properly; thanks for the reminder ;)). -- Catalin
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 1/27/20 7:33 PM, Qian Cai wrote: > >>> What’s the value of this block of new code? It only supports x86 and arm64 >>> which are supposed to be good now. >> We have been over the usefulness of this code many times before as the patch >> is >> already in it's V12. Currently it is enabled on arm64, x86 (except PAE), arc >> and >> ppc32. There are build time or runtime problems with other archs which >> prevent > I am not sure if I care too much about arc and ppc32 which are pretty much > legacy > platforms. You really need to brush up on your definition and knowledge of what "legacy" means. ARC is actively maintained and used by several customers, some in arch/arc/plat* and some not in there. It is present in broadband routers used by major ISP, massively multicore deep packet inspection system from EZChip, and many more Sure you may not care about them, but the maintainers for the platforms do. It would have been better if you had spent the time and energy in improving the code over 12 revisions rather than bike shedding. -Vineet
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
Le 28/01/2020 à 02:27, Anshuman Khandual a écrit :
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.
[...]
Tested-by: Christophe Leroy #PPC32
Also tested on PPC64 (under QEMU): book3s/64 64k pages, book3s/64 4k
pages and book3e/64
Reviewed-by: Ingo Molnar
Suggested-by: Catalin Marinas
Signed-off-by: Andrew Morton
Signed-off-by: Christophe Leroy
Signed-off-by: Anshuman Khandual
---
[...]
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 ..f3f8111edbe3
--- /dev/null
+++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt
@@ -0,0 +1,35 @@
+#
+# Feature name: debug-vm-pgtable
+# Kconfig: ARCH_HAS_DEBUG_VM_PGTABLE
+# description: arch supports pgtable tests for semantics compliance
+#
+---
+| arch |status|
+---
+| alpha: | TODO |
+| arc: | ok |
+| 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/32: | ok |
+| powerpc/64: | TODO |
You can change the two above lines by
powerpc: ok
+| riscv: | TODO |
+|s390: | TODO |
+| sh: | TODO |
+| sparc: | TODO |
+| um: | TODO |
+| unicore32: | TODO |
+| x86: | ok |
+| xtensa: | TODO |
+---
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 1ec34e16ed65..253dcab0bebc 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -120,6 +120,7 @@ config PPC
#
select ARCH_32BIT_OFF_T if PPC32
select ARCH_HAS_DEBUG_VIRTUAL
+ select ARCH_HAS_DEBUG_VM_PGTABLE if PPC32
Remove the 'if PPC32' as we now know it also work on PPC64.
select ARCH_HAS_DEVMEM_IS_ALLOWED
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FORTIFY_SOURCE
diff --git a/arch/x86/include/asm/pgtable_64.h
b/arch/x86/include/asm/pgtable_64.h
index 0b6c4042942a..fb0e76d254b3 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -53,6 +53,12 @@ static inline void sync_initial_page_table(void) { }
struct mm_struct;
+#define mm_p4d_folded mm_p4d_folded
+static inline bool mm_p4d_folded(struct mm_struct *mm)
+{
+ return !pgtable_l5_enabled();
+}
+
For me this should be part of another patch, it is not directly linked
to the tests.
void set_pte_vaddr_p4d(p4d_t *p4d_page, unsigned long vaddr, pte_t new_pte);
void set_pte_vaddr_pud(pud_t *pud_page, unsigned long vaddr, pte_t new_pte);
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index 798ea36a0549..e0b04787e789 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -1208,6 +1208,12 @@ static inline bool arch_has_pfn_modify_check(void)
# define PAGE_KERNEL_EXEC PAGE_KERNEL
#endif
+#ifdef CONFIG_DEBUG_VM_PGTABLE
Not sure it is a good idea to put that in include/asm-generic/pgtable.h
By doing this you are forcing a
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 28, 2020, at 7:10 AM, Mike Rapoport wrote: > > Aren't x86 and arm64 not decent enough? > Even if this test could be used to detect regressions only on these two > platforms, the test is valuable. The question is does it detect regressions good enough? Where is the list of past bugs that it had found? It is an usual deal for unproven debugging features remain out of tree first and keep gathering unique bugs it found and then justify for a mainline inclusion with enough data.
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
Hello Qian, On Mon, Jan 27, 2020 at 10:33:08PM -0500, Qian Cai wrote: > > > On Jan 27, 2020, at 10:06 PM, Anshuman Khandual > > wrote: > > > > enablement of this test (for the moment) but then the goal is to integrate > > all > > of them going forward. The test not only validates platform's adherence to > > the > > expected semantics from generic MM but also helps in keeping it that way > > during > > code changes in future as well. > > Another option maybe to get some decent arches on board first before merging > this > thing, so it have more changes to catch regressions for developers who might > run this. Aren't x86 and arm64 not decent enough? Even if this test could be used to detect regressions only on these two platforms, the test is valuable. -- Sincerely yours, Mike.
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On Tue, Jan 28, 2020 at 02:12:56AM -0500, Qian Cai wrote: > > On Jan 28, 2020, at 1:13 AM, Christophe Leroy > > wrote: > > ppc32 an indecent / legacy platform ? Are you kidying ? > > Powerquicc II PRO for instance is fully supported by the > > manufacturer and widely used in many small networking devices. > Of course I forgot about embedded devices. The problem is that how > many developers are actually going to run this debug option on > embedded devices? Much fewer if the code isn't upstream than if it is. This isn't something that every developer is going to enable all the time but that doesn't mean it's not useful, it's more for people doing work on the architectures or on memory management (or who suspect they're running into a relevant problem), and I'm sure some of the automated testing people will enable it. The more barriers there are in place to getting the testsuite up and running the less likely it is that any of these groups will run it regularly. signature.asc Description: PGP signature
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 28, 2020, at 1:13 AM, Christophe Leroy wrote: > > ppc32 an indecent / legacy platform ? Are you kidying ? > > Powerquicc II PRO for instance is fully supported by the manufacturer and > widely used in many small networking devices. Of course I forgot about embedded devices. The problem is that how many developers are actually going to run this debug option on embedded devices?
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 28, 2020, at 2:03 AM, Anshuman Khandual > wrote: > > 'allyesconfig' makes 'DEBUG_VM = y' which in turn will enable > 'DEBUG_VM_PGTABLE = y' > on platforms that subscribe ARCH_HAS_DEBUG_VM_PGTABLE. Isn’t that only for compiling testing? Who is booting such a beast and make sure everything working as expected?
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 01/28/2020 12:06 PM, Qian Cai wrote: > > >> On Jan 28, 2020, at 1:17 AM, Christophe Leroy >> wrote: >> >> It is 'default y' so there is no much risk that it is forgotten, at least >> all test suites run with 'allyes_defconfig' will trigger the test, so I >> think it is really a good feature. > > This thing depends on DEBUG_VM which I don’t see it is selected by any > defconfig. Am I missing anything? > 'allyesconfig' makes 'DEBUG_VM = y' which in turn will enable 'DEBUG_VM_PGTABLE = y' on platforms that subscribe ARCH_HAS_DEBUG_VM_PGTABLE.
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 28, 2020, at 1:17 AM, Christophe Leroy wrote: > > It is 'default y' so there is no much risk that it is forgotten, at least all > test suites run with 'allyes_defconfig' will trigger the test, so I think it > is really a good feature. This thing depends on DEBUG_VM which I don’t see it is selected by any defconfig. Am I missing anything?
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
Le 28/01/2020 à 06:48, Qian Cai a écrit : On Jan 27, 2020, at 11:58 PM, Anshuman Khandual wrote: As I had mentioned before, the test attempts to formalize page table helper semantics as expected from generic MM code paths and intend to catch deviations when enabled on a given platform. How else should we test semantics errors otherwise ? There are past examples of usefulness for this procedure on arm64 and on s390. I am wondering how else to prove the usefulness of a debug feature if these references are not enough. Not saying it will not be useful. As you mentioned it actually found a bug or two in the past. The problem is that there is always a cost to maintain something like this, and nobody knew how things could be broken even for the isolated code you mentioned in the future given how complicated the kernel code base is. I am not so positive that many developers would enable this debug feature and use it on a regular basis from the information you gave so far. On the other hand, it might just be good at maintaining this thing out of tree by yourself anyway, because if there isn’t going to be used by many developers, few people is going to contribute to this and even noticed when it is broken. What’s the point of getting this merged apart from being getting some meaningless credits? It is 'default y' so there is no much risk that it is forgotten, at least all test suites run with 'allyes_defconfig' will trigger the test, so I think it is really a good feature. Christophe
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
Le 28/01/2020 à 04:33, Qian Cai a écrit : On Jan 27, 2020, at 10:06 PM, Anshuman Khandual wrote: On 01/28/2020 07:41 AM, Qian Cai wrote: On Jan 27, 2020, at 8:28 PM, 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. Hello Qian, What’s the value of this block of new code? It only supports x86 and arm64 which are supposed to be good now. We have been over the usefulness of this code many times before as the patch is already in it's V12. Currently it is enabled on arm64, x86 (except PAE), arc and ppc32. There are build time or runtime problems with other archs which prevent I am not sure if I care too much about arc and ppc32 which are pretty much legacy platforms. enablement of this test (for the moment) but then the goal is to integrate all of them going forward. The test not only validates platform's adherence to the expected semantics from generic MM but also helps in keeping it that way during code changes in future as well. Another option maybe to get some decent arches on board first before merging this thing, so it have more changes to catch regressions for developers who might run this. ppc32 an indecent / legacy platform ? Are you kidying ? Powerquicc II PRO for instance is fully supported by the manufacturer and widely used in many small networking devices. Christophe
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 27, 2020, at 11:58 PM, Anshuman Khandual > wrote: > > As I had mentioned before, the test attempts to formalize page table helper > semantics > as expected from generic MM code paths and intend to catch deviations when > enabled on > a given platform. How else should we test semantics errors otherwise ? There > are past > examples of usefulness for this procedure on arm64 and on s390. I am > wondering how > else to prove the usefulness of a debug feature if these references are not > enough. Not saying it will not be useful. As you mentioned it actually found a bug or two in the past. The problem is that there is always a cost to maintain something like this, and nobody knew how things could be broken even for the isolated code you mentioned in the future given how complicated the kernel code base is. I am not so positive that many developers would enable this debug feature and use it on a regular basis from the information you gave so far. On the other hand, it might just be good at maintaining this thing out of tree by yourself anyway, because if there isn’t going to be used by many developers, few people is going to contribute to this and even noticed when it is broken. What’s the point of getting this merged apart from being getting some meaningless credits?
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 01/28/2020 09:03 AM, Qian Cai wrote: > > >> On Jan 27, 2020, at 10:06 PM, Anshuman Khandual >> wrote: >> >> >> >> On 01/28/2020 07:41 AM, Qian Cai wrote: >>> >>> On Jan 27, 2020, at 8:28 PM, 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. >> >> Hello Qian, >> >>> >>> What’s the value of this block of new code? It only supports x86 and arm64 >>> which are supposed to be good now. >> >> We have been over the usefulness of this code many times before as the patch >> is >> already in it's V12. Currently it is enabled on arm64, x86 (except PAE), arc >> and >> ppc32. There are build time or runtime problems with other archs which >> prevent > > I am not sure if I care too much about arc and ppc32 which are pretty much > legacy > platforms. Okay but FWIW the maintainers for all these enabled platforms cared for this test at the least and really helped in shaping the test to it's current state. Besides I am still failing to understand your point here about evaluating particular feature's usefulness based on it's support on relative and perceived importance of some platforms compared to others. Again the idea is to integrate all platforms eventually but we had discovered build and runtime issues which needs to be resolved at platform level first. Unless I am mistaken, debug feature like this which is putting down a framework while also benefiting some initial platforms to start with, will be a potential candidate for eventual inclusion in the mainline. Otherwise, please point to any other agreed upon community criteria for debug feature's mainline inclusion which I will try to adhere. I wonder if all other similar debug features from the past ever met 'the all inclusive at the beginning' criteria which you are trying to propose here. This test also adds a feature file, enlisting all supported archs as suggested by Ingo for the exact same reason. This is not the first time, a feature is listing out archs which are supported and archs which are not. > >> enablement of this test (for the moment) but then the goal is to integrate >> all >> of them going forward. The test not only validates platform's adherence to >> the >> expected semantics from generic MM but also helps in keeping it that way >> during >> code changes in future as well. > > Another option maybe to get some decent arches on board first before merging > this > thing, so it have more changes to catch regressions for developers who might > run this. > >> >>> Did those tests ever find any regression or this is almost only useful for >>> new >> >> The test has already found problems with s390 page table helpers. > > Hmm, that is pretty weak where s390 is not even official supported with this > version. And there were valid reasons why s390 could not be enabled just yet as explained by s390 folks during our previous discussions. I just pointed out an example where this test was useful as you had asked previously. Not being official supported in this version does not take away the fact the it was indeed useful for that platform in discovering a bug. > >> >>> architectures which only happened once in a few years? >> >> Again, not only it validates what exist today but its also a tool to make >> sure that all platforms continue adhere to a common agreed upon semantics >> as reflected through the tests here. >> >>> The worry if not many people will use this config and code those that much >>> in >> >> Debug features or tests in the kernel are used when required. These are >> never or >> should not be enabled by default. AFAICT this is true even for entire >> DEBUG_VM >> packaged tests. Do you have any particular data or precedence to substantiate >> the fact that this test will be used any less often than the
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 27, 2020, at 10:06 PM, Anshuman Khandual > wrote: > > > > On 01/28/2020 07:41 AM, Qian Cai wrote: >> >> >>> On Jan 27, 2020, at 8:28 PM, 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. > > Hello Qian, > >> >> What’s the value of this block of new code? It only supports x86 and arm64 >> which are supposed to be good now. > > We have been over the usefulness of this code many times before as the patch > is > already in it's V12. Currently it is enabled on arm64, x86 (except PAE), arc > and > ppc32. There are build time or runtime problems with other archs which prevent I am not sure if I care too much about arc and ppc32 which are pretty much legacy platforms. > enablement of this test (for the moment) but then the goal is to integrate all > of them going forward. The test not only validates platform's adherence to the > expected semantics from generic MM but also helps in keeping it that way > during > code changes in future as well. Another option maybe to get some decent arches on board first before merging this thing, so it have more changes to catch regressions for developers who might run this. > >> Did those tests ever find any regression or this is almost only useful for >> new > > The test has already found problems with s390 page table helpers. Hmm, that is pretty weak where s390 is not even official supported with this version. > >> architectures which only happened once in a few years? > > Again, not only it validates what exist today but its also a tool to make > sure that all platforms continue adhere to a common agreed upon semantics > as reflected through the tests here. > >> The worry if not many people will use this config and code those that much in > > Debug features or tests in the kernel are used when required. These are never > or > should not be enabled by default. AFAICT this is true even for entire DEBUG_VM > packaged tests. Do you have any particular data or precedence to substantiate > the fact that this test will be used any less often than the other similar > ones > in the tree ? I can only speak for arm64 platform but the very idea for this > test came from Catalin when we were trying to understand the semantics for THP > helpers while enabling THP migration without split. Apart from going over the > commit messages from the past, there were no other way to figure out how any > particular page table helper is suppose to change given page table entry. This > test tries to formalize those semantics. I am thinking about how we made so many mistakes before by merging too many of those debugging options that many of them have been broken for many releases proving that nobody actually used them regularly. We don’t need to repeat the same mistake again. I am actually thinking about to remove things like page_poisoning often which is almost are never found any bug recently and only cause pains when interacting with other new features that almost nobody will test them together to begin with. We even have some SLUB debugging code sit there for almost 15 years that almost nobody used it and maintainers refused to remove it. > >> the future because it is inefficient to find bugs, it will simply be rotten > Could you be more specific here ? What parts of the test are inefficient ? I > am happy to improve upon the test. Do let me know you if you have suggestions. > >> like a few other debugging options out there we have in the mainline that > will be a pain to remove later on. >> > > Even though I am not agreeing to your assessment about the usefulness of the > test without any substantial data backing up the claims, the test case in > itself is very much compartmentalized, staying clear from generic MM and > debug_vm_pgtable() is only function executing the test
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
On 01/28/2020 07:41 AM, Qian Cai wrote: > > >> On Jan 27, 2020, at 8:28 PM, 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. Hello Qian, > > What’s the value of this block of new code? It only supports x86 and arm64 > which are supposed to be good now. We have been over the usefulness of this code many times before as the patch is already in it's V12. Currently it is enabled on arm64, x86 (except PAE), arc and ppc32. There are build time or runtime problems with other archs which prevent enablement of this test (for the moment) but then the goal is to integrate all of them going forward. The test not only validates platform's adherence to the expected semantics from generic MM but also helps in keeping it that way during code changes in future as well. > Did those tests ever find any regression or this is almost only useful for new The test has already found problems with s390 page table helpers. > architectures which only happened once in a few years? Again, not only it validates what exist today but its also a tool to make sure that all platforms continue adhere to a common agreed upon semantics as reflected through the tests here. > The worry if not many people will use this config and code those that much in Debug features or tests in the kernel are used when required. These are never or should not be enabled by default. AFAICT this is true even for entire DEBUG_VM packaged tests. Do you have any particular data or precedence to substantiate the fact that this test will be used any less often than the other similar ones in the tree ? I can only speak for arm64 platform but the very idea for this test came from Catalin when we were trying to understand the semantics for THP helpers while enabling THP migration without split. Apart from going over the commit messages from the past, there were no other way to figure out how any particular page table helper is suppose to change given page table entry. This test tries to formalize those semantics. > the future because it is inefficient to find bugs, it will simply be rotten Could you be more specific here ? What parts of the test are inefficient ? I am happy to improve upon the test. Do let me know you if you have suggestions. > like a few other debugging options out there we have in the mainline that will be a pain to remove later on. > Even though I am not agreeing to your assessment about the usefulness of the test without any substantial data backing up the claims, the test case in itself is very much compartmentalized, staying clear from generic MM and debug_vm_pgtable() is only function executing the test which is getting called from kernel_init_freeable() path. - Anshuman
Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
> On Jan 27, 2020, at 8:28 PM, 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. What’s the value of this block of new code? It only supports x86 and arm64 which are supposed to be good now. Did those tests ever find any regression or this is almost only useful for new architectures which only happened once in a few years? The worry if not many people will use this config and code those that much in the future because it is inefficient to find bugs, it will simply be rotten like a few other debugging options out there we have in the mainline that will be a pain to remove later on.
