From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DD619C3524A for ; Sun, 2 Feb 2020 07:18:55 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 898ED20679 for ; Sun, 2 Feb 2020 07:18:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 898ED20679 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 35E256B0624; Sun, 2 Feb 2020 02:18:55 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 335726B0625; Sun, 2 Feb 2020 02:18:55 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 24BE66B0626; Sun, 2 Feb 2020 02:18:55 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0184.hostedemail.com [216.40.44.184]) by kanga.kvack.org (Postfix) with ESMTP id 0D2E46B0624 for ; Sun, 2 Feb 2020 02:18:55 -0500 (EST) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 9A7FA2489 for ; Sun, 2 Feb 2020 07:18:54 +0000 (UTC) X-FDA: 76444334988.22.room29_87d8f160a6556 X-HE-Tag: room29_87d8f160a6556 X-Filterd-Recvd-Size: 11977 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf26.hostedemail.com (Postfix) with ESMTP for ; Sun, 2 Feb 2020 07:18:53 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6E295FEC; Sat, 1 Feb 2020 23:18:52 -0800 (PST) Received: from [10.163.1.42] (unknown [10.163.1.42]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 901DF3F68E; Sat, 1 Feb 2020 23:22:10 -0800 (PST) Subject: Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers To: Christophe Leroy , linux-mm@kvack.org Cc: Andrew Morton , Vlastimil Babka , Greg Kroah-Hartman , Thomas Gleixner , Mike Rapoport , Jason Gunthorpe , Dan Williams , Peter Zijlstra , Michal Hocko , Mark Rutland , Mark Brown , Steven Price , Ard Biesheuvel , Masahiro Yamada , Kees Cook , Tetsuo Handa , Matthew Wilcox , Sri Krishna chowdary , Dave Hansen , Russell King - ARM Linux , Michael Ellerman , Paul Mackerras , Martin Schwidefsky , Heiko Carstens , "David S. Miller" , Vineet Gupta , James Hogan , Paul Burton , Ralf Baechle , "Kirill A . Shutemov" , Gerald Schaefer , Ingo Molnar , linux-snps-arc@lists.infradead.org, linux-mips@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-ia64@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, sparclinux@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org References: <1580174873-18117-1-git-send-email-anshuman.khandual@arm.com> <68ed6488-aa25-ab41-8da6-f0ddeb15d52b@c-s.fr> <49754f74-53a7-0e4a-bb16-53617f8c902c@arm.com> <473d8198-3ac4-af3b-e2ec-c0698a3565d3@c-s.fr> From: Anshuman Khandual Message-ID: Date: Sun, 2 Feb 2020 12:48:29 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <473d8198-3ac4-af3b-e2ec-c0698a3565d3@c-s.fr> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 01/30/2020 07:43 PM, Christophe Leroy wrote: >=20 >=20 > Le 30/01/2020 =C3=A0 14:04, Anshuman Khandual a =C3=A9crit=C2=A0: >> >> On 01/28/2020 10:35 PM, Christophe Leroy wrote: >>> >>> >>> Le 28/01/2020 =C3=A0 02:27, Anshuman Khandual a =C3=A9crit=C2=A0: >>>> diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/as= m/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) = { } >>>> =C2=A0=C2=A0 =C2=A0 struct mm_struct; >>>> =C2=A0=C2=A0 +#define mm_p4d_folded mm_p4d_folded >>>> +static inline bool mm_p4d_folded(struct mm_struct *mm) >>>> +{ >>>> +=C2=A0=C2=A0=C2=A0 return !pgtable_l5_enabled(); >>>> +} >>>> + >>> >>> For me this should be part of another patch, it is not directly linke= d 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.rkds4f3fqv3yjh= jy@box/ >=20 > 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. >=20 > But that's up to you. >=20 >> >>> >>>> =C2=A0=C2=A0 void set_pte_vaddr_p4d(p4d_t *p4d_page, unsigned long v= addr, pte_t new_pte); >>>> =C2=A0=C2=A0 void set_pte_vaddr_pud(pud_t *pud_page, unsigned long v= addr, pte_t new_pte); >>>> =C2=A0=C2=A0 diff --git a/include/asm-generic/pgtable.h b/include/as= m-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) >>>> =C2=A0=C2=A0 # define PAGE_KERNEL_EXEC PAGE_KERNEL >>>> =C2=A0=C2=A0 #endif >>>> =C2=A0=C2=A0 +#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. >=20 > I can't see any debug related features in that file. >=20 >> >>> >>> 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 activat= ing this config option. >> >> I agreed but whats the alternative ? We could move these into init/mai= n.c >> to make things simpler but will that be a right place, given its relat= ed >> to generic page table. >=20 > What about linux/mmdebug.h instead ? (I have not checked if it would re= duce the impact, but that's where things related to CONFIG_DEBUG_VM seems= to be). >=20 > Otherwise, you can just create new file, for instance and include that file only in the init/main.c and mm/debug_vm_pg= table.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_PGFLAG= S, DEBUG_VIRTUAL (which is also a stand alone test). A simple grep shows tha= t the impact of mmdebug.h would be less than generic pgtable.h header. >=20 >=20 >=20 >> >>> >>>> +extern void debug_vm_pgtable(void); >>> >>> Please don't use the 'extern' keyword, it is useless and not to be us= ed for functions declaration. >> >> Really ? But, there are tons of examples doing the same thing both in >> generic and platform code as well. >=20 > 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. >=20 > I think checkpatch.pl usually complains when a patch brings a new unrel= eval extern symbol. Sure np, will drop it. But checkpatch.pl never complained. >=20 >> >>> >>>> +#else >>>> +static inline void debug_vm_pgtable(void) { } >>>> +#endif >>>> + >>>> =C2=A0=C2=A0 #endif /* !__ASSEMBLY__ */ >>>> =C2=A0=C2=A0 =C2=A0 #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_freeab= le(void) >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sched_init_smp(); >>>> =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page_alloc_init_late(); >>>> +=C2=A0=C2=A0=C2=A0 debug_vm_pgtable(); >>> >>> Wouldn't it be better to call debug_vm_pgtable() in kernel_init() bet= ween 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(= ) ? >=20 > It would avoid having it lost in the middle of drivers logs, would be c= lose 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. >=20 > 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. >=20 >> >>> >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Initialize page ext after al= l struct pages are initialized. */ >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page_ext_init(); >>>> =C2=A0=C2=A0 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 >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 data corruption or = a sporadic crash at a later stage once the region >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 is examined. The ru= ntime overhead introduced is minimal. >>>> =C2=A0=C2=A0 +config ARCH_HAS_DEBUG_VM_PGTABLE >>>> +=C2=A0=C2=A0=C2=A0 bool >>>> +=C2=A0=C2=A0=C2=A0 help >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 An architecture should select this w= hen it can successfully >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 build and run DEBUG_VM_PGTABLE. >>>> + >>>> =C2=A0=C2=A0 config DEBUG_VM >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bool "Debug VM" >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 depends on DEBUG_KERNEL >>>> @@ -688,6 +694,22 @@ config DEBUG_VM_PGFLAGS >>>> =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 If unsure, s= ay N. >>>> =C2=A0=C2=A0 +config DEBUG_VM_PGTABLE >>>> +=C2=A0=C2=A0=C2=A0 bool "Debug arch page table for semantics compli= ance" >>>> +=C2=A0=C2=A0=C2=A0 depends on MMU >>>> +=C2=A0=C2=A0=C2=A0 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 (althoug= h I >> dont remember the conversation around it) and hence this dependency. >=20 > Yes but it perfectly work as standalone. The more easy it is to activat= e 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 testin= g behave differently when DEBUG_VM is not set ? I think it's good the tes= t things as close as possible to final config. Makes sense. There is no functional dependency for the individual tests here on DEBUG_VM. >=20 >> >>> I think we could make it standalone and 'default y if DEBUG_VM' inste= ad. >> >> Which will yield the same result like before but in a different way. B= ut >> yes, this test could go about either way but unless there is a good en= ough >> reason why change the current one. >=20 > I think if we want people to really use it on other architectures it mu= st be possible to activate it without having to modify Kconfig. Otherwise= people won't even know the test exists and the architecture fails the te= st. >=20 > The purpose of a test suite is to detect bugs. If you can't run the tes= t until you have fixed the bugs, I guess nobody will ever detect the bugs= and they will never be fixed. >=20 > So I think: > - the test should be 'default y' when ARCH_HAS_DEBUG_VM_PGTABLE is sele= cted > - 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. >=20 > Something like: >=20 > config DEBUG_VM_PGTABLE > =C2=A0=C2=A0=C2=A0 bool "Debug arch page table for semantics compliance= " if ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT > =C2=A0=C2=A0=C2=A0 depends on MMU (ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT) be moved along side MMU on the same= line ? > =C2=A0=C2=A0=C2=A0 default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE > =C2=A0=C2=A0=C2=A0 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. >=20 >=20 >> >>> >>>> +=C2=A0=C2=A0=C2=A0 depends on ARCH_HAS_DEBUG_VM_PGTABLE >>>> +=C2=A0=C2=A0=C2=A0 default y >>>> +=C2=A0=C2=A0=C2=A0 help >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 This option provides a debug method = which can be used to test >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 architecture page table helper funct= ions on various platforms in >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 verifying if they comply with expect= ed generic MM semantics. This >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 will help architecture code in makin= g sure that any changes or >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 new additions of these helpers still= conform to expected >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 semantics of the generic MM. >>>> + >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 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 la= st >> sentence "If unsure, say N". Nice catch, thank you. >=20 > Well I was not asking if 'default y' was making sense but only if 'If u= nsure say N' was making sense due to the 'default y'. You got it. >=20 > Christophe >=20 >=20