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
>
>
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
