Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers

[Anshuman Khandual]

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 <linux/mmdebug-pgtable.h> 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 <linux/mmdebug.h> 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
>>>> @@ -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 but it perfectly work as standalone. The more easy it is to activate and the more people will use it. DEBUG_VM obliges to rebuild the kernel entirely and could modify the behaviour. Could the helpers we are testing behave differently when DEBUG_VM is not set ? I think it's good the test things as close as possible to final config.

Makes sense. There is no functional dependency for the individual tests
here on DEBUG_VM.

> 
>>
>>> 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 ?

> ÂÂÂ 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.

> 
> 
>>
>>>
>>>> +ÂÂÂ depends on ARCH_HAS_DEBUG_VM_PGTABLE
>>>> +ÂÂÂ default y
>>>> +ÂÂÂ help
>>>> +ÂÂÂÂÂ This option provides a debug method which can be used to test
>>>> +ÂÂÂÂÂ architecture page table helper functions on various platforms in
>>>> +ÂÂÂÂÂ verifying if they comply with expected generic MM semantics. This
>>>> +ÂÂÂÂÂ will help architecture code in making sure that any changes or
>>>> +ÂÂÂÂÂ new additions of these helpers still conform to expected
>>>> +ÂÂÂÂÂ semantics of the generic MM.
>>>> +
>>>> +ÂÂÂÂÂ If unsure, say N.
>>>> +
>>>
>>> Does it make sense to make it 'default y' and say 'If unsure, say N' ?
>>
>> No it does. Not when it defaults 'y' unconditionally. Will drop the last
>> sentence "If unsure, say N". Nice catch, thank you.
> 
> Well I was not asking if 'default y' was making sense but only if 'If unsure say N' was making sense due to the 'default y'. You got it.
> 
> Christophe
> 
>