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.0 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,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 9AF85C35240 for ; Thu, 30 Jan 2020 14:13:23 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5128C215A4 for ; Thu, 30 Jan 2020 14:13:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=c-s.fr header.i=@c-s.fr header.b="qICZOrjf" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5128C215A4 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=c-s.fr Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id D95D06B035A; Thu, 30 Jan 2020 09:13:22 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D6C9B6B035B; Thu, 30 Jan 2020 09:13:22 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C83A46B035C; Thu, 30 Jan 2020 09:13:22 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0114.hostedemail.com [216.40.44.114]) by kanga.kvack.org (Postfix) with ESMTP id B460C6B035A for ; Thu, 30 Jan 2020 09:13:22 -0500 (EST) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 777A7180AD802 for ; Thu, 30 Jan 2020 14:13:22 +0000 (UTC) X-FDA: 76434493044.04.war91_57856b0758b49 X-HE-Tag: war91_57856b0758b49 X-Filterd-Recvd-Size: 12296 Received: from pegase1.c-s.fr (pegase1.c-s.fr [93.17.236.30]) by imf24.hostedemail.com (Postfix) with ESMTP for ; Thu, 30 Jan 2020 14:13:21 +0000 (UTC) Received: from localhost (mailhub1-int [192.168.12.234]) by localhost (Postfix) with ESMTP id 487j4Z2bp9z9v2Nx; Thu, 30 Jan 2020 15:13:18 +0100 (CET) Authentication-Results: localhost; dkim=pass reason="1024-bit key; insecure key" header.d=c-s.fr header.i=@c-s.fr header.b=qICZOrjf; dkim-adsp=pass; dkim-atps=neutral X-Virus-Scanned: Debian amavisd-new at c-s.fr Received: from pegase1.c-s.fr ([192.168.12.234]) by localhost (pegase1.c-s.fr [192.168.12.234]) (amavisd-new, port 10024) with ESMTP id uOkb5XiF-zPA; Thu, 30 Jan 2020 15:13:18 +0100 (CET) Received: from messagerie.si.c-s.fr (messagerie.si.c-s.fr [192.168.25.192]) by pegase1.c-s.fr (Postfix) with ESMTP id 487j4Z13FJz9v2Nw; Thu, 30 Jan 2020 15:13:18 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=c-s.fr; s=mail; t=1580393598; bh=jiy8q+DLF/cyz8x0j+2VD0K/r6LzapCGUAMM8PPwAqg=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=qICZOrjfjgKU41D9G6qwOhWQ3f07+KjLwpAHo5DoCMTyruxB3bymqHi8naihrjRrd VNgNmAI2BgWTRJKP0fo15DiJvJYT/IAWdqQGaK6fSFCsfnDbh4zU2Oj9aR7VXIDKWw S5BQleimwSbi1BQRePhLQirIhOLqknAgD+VG9wXI= Received: from localhost (localhost [127.0.0.1]) by messagerie.si.c-s.fr (Postfix) with ESMTP id 606438B875; Thu, 30 Jan 2020 15:13:19 +0100 (CET) X-Virus-Scanned: amavisd-new at c-s.fr Received: from messagerie.si.c-s.fr ([127.0.0.1]) by localhost (messagerie.si.c-s.fr [127.0.0.1]) (amavisd-new, port 10023) with ESMTP id v9YV4qhIXl9S; Thu, 30 Jan 2020 15:13:19 +0100 (CET) Received: from [192.168.4.90] (unknown [192.168.4.90]) by messagerie.si.c-s.fr (Postfix) with ESMTP id CC2F68B874; Thu, 30 Jan 2020 15:13:16 +0100 (CET) Subject: Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers To: Anshuman Khandual , 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> From: Christophe Leroy Message-ID: <473d8198-3ac4-af3b-e2ec-c0698a3565d3@c-s.fr> Date: Thu, 30 Jan 2020 15:13:16 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.4.2 MIME-Version: 1.0 In-Reply-To: <49754f74-53a7-0e4a-bb16-53617f8c902c@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: fr 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: Le 30/01/2020 =C3=A0 14:04, Anshuman Khandual a =C3=A9crit=C2=A0: >=20 > 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/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) {= } >>> =C2=A0 =C2=A0 struct mm_struct; >>> =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 linked= to the tests. >=20 > We did discuss about this earlier and Kirril mentioned its not worth > a separate patch. >=20 > https://lore.kernel.org/linux-arm-kernel/20190913091305.rkds4f3fqv3yjhj= y@box/ For me it would make sense to not mix this patch which implement tests,=20 and changes that are needed for the test to work (or even build) on the=20 different architectures. But that's up to you. >=20 >> >>> =C2=A0 void set_pte_vaddr_p4d(p4d_t *p4d_page, unsigned long vaddr, = pte_t new_pte); >>> =C2=A0 void set_pte_vaddr_pud(pud_t *pud_page, unsigned long vaddr, = pte_t new_pte); >>> =C2=A0 diff --git a/include/asm-generic/pgtable.h b/include/asm-gene= ric/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(v= oid) >>> =C2=A0 # define PAGE_KERNEL_EXEC PAGE_KERNEL >>> =C2=A0 #endif >>> =C2=A0 +#ifdef CONFIG_DEBUG_VM_PGTABLE >> >> Not sure it is a good idea to put that in include/asm-generic/pgtable.= h >=20 > 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. >=20 >> >> By doing this you are forcing a rebuild of almost all files, whereas o= nly init/main.o and mm/debug_vm_pgtable.o should be rebuilt when activati= ng this config option. >=20 > 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 relate= d > to generic page table. What about linux/mmdebug.h instead ? (I have not checked if it would=20 reduce the impact, but that's where things related to CONFIG_DEBUG_VM=20 seems to be). Otherwise, you can just create new file, for instance=20 and include that file only in the init/main.c=20 and mm/debug_vm_pgtable.c >=20 >> >>> +extern void debug_vm_pgtable(void); >> >> Please don't use the 'extern' keyword, it is useless and not to be use= d for functions declaration. >=20 > 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=20 ? Having tons of 'extern' doesn't mean we must add more. I think checkpatch.pl usually complains when a patch brings a new=20 unreleval extern symbol. >=20 >> >>> +#else >>> +static inline void debug_vm_pgtable(void) { } >>> +#endif >>> + >>> =C2=A0 #endif /* !__ASSEMBLY__ */ >>> =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_freeabl= e(void) >>> =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 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() betw= een the call to async_synchronise_full() and ftrace_free_init_mem() ? >=20 > 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=20 close to the end of init, at a place we can't miss it, close to the=20 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=20 what the result is. >=20 >> >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Initialize page ext after all stru= ct pages are initialized. */ >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page_ext_init(); >>> =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 data corruption or a spor= adic crash at a later stage once the region >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 is examined. The runtime = overhead introduced is minimal. >>> =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 wh= en it can successfully >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 build and run DEBUG_VM_PGTABLE. >>> + >>> =C2=A0 config DEBUG_VM >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bool "Debug VM" >>> =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 If unsure, say N. >>> =C2=A0 +config DEBUG_VM_PGTABLE >>> +=C2=A0=C2=A0=C2=A0 bool "Debug arch page table for semantics complia= nce" >>> +=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 ? >=20 > 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=20 and the more people will use it. DEBUG_VM obliges to rebuild the kernel=20 entirely and could modify the behaviour. Could the helpers we are=20 testing behave differently when DEBUG_VM is not set ? I think it's good=20 the test things as close as possible to final config. >=20 >> I think we could make it standalone and 'default y if DEBUG_VM' instea= d. >=20 > Which will yield the same result like before but in a different way. Bu= t > yes, this test could go about either way but unless there is a good eno= ugh > reason why change the current one. I think if we want people to really use it on other architectures it=20 must be possible to activate it without having to modify Kconfig.=20 Otherwise people won't even know the test exists and the architecture=20 fails the test. The purpose of a test suite is to detect bugs. If you can't run the test=20 until you have fixed the bugs, I guess nobody will ever detect the bugs=20 and they will never be fixed. So I think: - the test should be 'default y' when ARCH_HAS_DEBUG_VM_PGTABLE is select= ed - the test should be 'default n' when ARCH_HAS_DEBUG_VM_PGTABLE is not=20 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=20 ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT depends on MMU default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE default 'y' if DEBUG_VM >=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 w= hich can be used to test >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 architecture page table helper functi= ons on various platforms in >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 verifying if they comply with expecte= d generic MM semantics. This >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 will help architecture code in making= 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' ? >=20 > No it does. Not when it defaults 'y' unconditionally. Will drop the las= t > sentence "If unsure, say N". Nice catch, thank you. Well I was not asking if 'default y' was making sense but only if 'If=20 unsure say N' was making sense due to the 'default y'. You got it. Christophe