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=-2.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no 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 E88DBC4CEC4 for ; Thu, 19 Sep 2019 05:41:51 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8C54F21907 for ; Thu, 19 Sep 2019 05:41:51 +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="P/9E7NxT" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8C54F21907 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 027676B0336; Thu, 19 Sep 2019 01:41:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F1B1C6B0337; Thu, 19 Sep 2019 01:41:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DE2556B0339; Thu, 19 Sep 2019 01:41:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0112.hostedemail.com [216.40.44.112]) by kanga.kvack.org (Postfix) with ESMTP id B7E086B0336 for ; Thu, 19 Sep 2019 01:41:50 -0400 (EDT) Received: from smtpin18.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with SMTP id 697E9610E for ; Thu, 19 Sep 2019 05:41:50 +0000 (UTC) X-FDA: 75950573580.18.rail97_674b77d5f6160 X-HE-Tag: rail97_674b77d5f6160 X-Filterd-Recvd-Size: 10848 Received: from pegase1.c-s.fr (pegase1.c-s.fr [93.17.236.30]) by imf20.hostedemail.com (Postfix) with ESMTP for ; Thu, 19 Sep 2019 05:41:49 +0000 (UTC) Received: from localhost (mailhub1-int [192.168.12.234]) by localhost (Postfix) with ESMTP id 46Ym1l1njQz9v1TZ; Thu, 19 Sep 2019 07:41:47 +0200 (CEST) Authentication-Results: localhost; dkim=pass reason="1024-bit key; insecure key" header.d=c-s.fr header.i=@c-s.fr header.b=P/9E7NxT; 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 g74_nX2HQhCp; Thu, 19 Sep 2019 07:41:47 +0200 (CEST) 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 46Ym1k6lm3z9v1D2; Thu, 19 Sep 2019 07:41:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=c-s.fr; s=mail; t=1568871706; bh=CyfWszO2OTTHF7lXp4NH0Xaf7qGkIMwgP9l9wpOQU4E=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=P/9E7NxTuWfgrzj1efsR+MbDalvbBxKgeu6vwUr4V5nDSpL9znIl/crMp+XoqqJQf garOuYl7V2QIaxxNQK3+Hc3/84xagK5/lk0efPBDGFbTsPL/ymC6NKmMor+2Ue78Yq Afx/sONO3EULCBSntmdxGswoTJGqlGydMST5VtTo= Received: from localhost (localhost [127.0.0.1]) by messagerie.si.c-s.fr (Postfix) with ESMTP id BD26A8B80C; Thu, 19 Sep 2019 07:41:47 +0200 (CEST) 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 vGu-ud8Pyz6Y; Thu, 19 Sep 2019 07:41:47 +0200 (CEST) Received: from [192.168.4.90] (unknown [192.168.4.90]) by messagerie.si.c-s.fr (Postfix) with ESMTP id 63BFE8B783; Thu, 19 Sep 2019 07:41:45 +0200 (CEST) Subject: Re: [PATCH V2 2/2] mm/pgtable/debug: Add test 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 , 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: <1568268173-31302-1-git-send-email-anshuman.khandual@arm.com> <1568268173-31302-3-git-send-email-anshuman.khandual@arm.com> <502c497a-9bf1-7d2e-95f2-cfebcd9cf1d9@arm.com> <95ed9d92-dd43-4c45-2e52-738aed7f2fb5@c-s.fr> <64504101-d9dd-f273-02f9-e9a8b178eecc@c-s.fr> <955491d9-d8aa-0a93-4fb9-3d15acfbcbf8@arm.com> From: Christophe Leroy Message-ID: Date: Thu, 19 Sep 2019 07:41:45 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <955491d9-d8aa-0a93-4fb9-3d15acfbcbf8@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 19/09/2019 =C3=A0 06:56, Anshuman Khandual a =C3=A9crit=C2=A0: >=20 >=20 > On 09/18/2019 09:56 PM, Christophe Leroy wrote: >> >> >> Le 18/09/2019 =C3=A0 07:04, Anshuman Khandual a =C3=A9crit=C2=A0: >>> >>> >>> On 09/13/2019 03:31 PM, Christophe Leroy wrote: >>>> >>>> >>>> Le 13/09/2019 =C3=A0 11:02, Anshuman Khandual a =C3=A9crit=C2=A0: >>>>> >>>>>>> +#if !defined(__PAGETABLE_PMD_FOLDED) && !defined(__ARCH_HAS_4LEV= EL_HACK) >>>>>> >>>>>> #ifdefs have to be avoided as much as possible, see below >>>>> >>>>> Yeah but it has been bit difficult to avoid all these $ifdef becaus= e of the >>>>> availability (or lack of it) for all these pgtable helpers in vario= us config >>>>> combinations on all platforms. >>>> >>>> As far as I can see these pgtable helpers should exist everywhere at= least via asm-generic/ files. >>> >>> But they might not actually do the right thing. >>> >>>> >>>> Can you spot a particular config which fails ? >>> >>> Lets consider the following example (after removing the $ifdefs aroun= d it) >>> which though builds successfully but fails to pass the intended test.= This >>> is with arm64 config 4K pages sizes with 39 bits VA space which ends = up >>> with a 3 level page table arrangement. >>> >>> static void __init p4d_clear_tests(p4d_t *p4dp) >>> { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p4d_t p4d =3D READ_= ONCE(*p4dp); >> >> My suggestion was not to completely drop the #ifdef but to do like you= did in pgd_clear_tests() for instance, ie to add the following test on t= op of the function: >> >> =C2=A0=C2=A0=C2=A0=C2=A0if (mm_pud_folded(mm) || is_defined(__ARCH_HA= S_5LEVEL_HACK)) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; >> >=20 > Sometimes this does not really work. On some platforms, combination of > __PAGETABLE_PUD_FOLDED and __ARCH_HAS_5LEVEL_HACK decide whether the > helpers such as __pud() or __pgd() is even available for that platform. > Ideally it should have been through generic falls backs in include/*/ > but I guess there might be bugs on the platform or it has not been > changed to adopt 5 level page table framework with required folding > macros etc. Yes. As I suggested below, most likely that's better to retain the=20 #ifdef __ARCH_HAS_5LEVEL_HACK but change the #ifdef=20 __PAGETABLE_PUD_FOLDED by a runtime test of mm_pud_folded(mm) As pointed by Gerald, some arches don't have __PAGETABLE_PUD_FOLDED=20 because they are deciding dynamically if they fold the level on not, but=20 have mm_pud_folded(mm) >=20 >>> >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p4d =3D __p4d(p4d_v= al(p4d) | RANDOM_ORVALUE); >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 WRITE_ONCE(*p4dp, p= 4d); >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p4d_clear(p4dp); >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p4d =3D READ_ONCE(*= p4dp); >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 WARN_ON(!p4d_none(p= 4d)); >>> } >>> >>> The following test hits an error at WARN_ON(!p4d_none(p4d)) >>> >>> [=C2=A0=C2=A0 16.757333] ------------[ cut here ]------------ >>> [=C2=A0=C2=A0 16.758019] WARNING: CPU: 11 PID: 1 at mm/arch_pgtable_t= est.c:187 arch_pgtable_tests_init+0x24c/0x474 [...] >>> [=C2=A0=C2=A0 16.781282] ---[ end trace 042e6c40c0a3b038 ]--- >>> >>> On arm64 (4K page size|39 bits VA|3 level page table) >>> >>> #elif CONFIG_PGTABLE_LEVELS =3D=3D 3=C2=A0=C2=A0=C2=A0 /* Applicable = here */ >>> #define __ARCH_USE_5LEVEL_HACK >>> #include >>> >>> Which pulls in >>> >>> #include >>> >>> which pulls in >>> >>> #include >>> >>> which defines >>> >>> static inline int p4d_none(p4d_t p4d) >>> { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0; >>> } >>> >>> which will invariably trigger WARN_ON(!p4d_none(p4d)). >>> >>> Similarly for next test p4d_populate_tests() which will always be >>> successful because p4d_bad() invariably returns negative. >>> >>> static inline int p4d_bad(p4d_t p4d) >>> { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0; >>> } >>> >>> static void __init p4d_populate_tests(struct mm_struct *mm, p4d_t *p4= dp, >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 pud_t *pudp) >>> { >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p4d_t p4d; >>> >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * This entry = points to next level page table page. >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Hence this = must not qualify as p4d_bad(). >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */ >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pud_clear(pudp); >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p4d_clear(p4dp); >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p4d_populate(mm, p4= dp, pudp); >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p4d =3D READ_ONCE(*= p4dp); >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 WARN_ON(p4d_bad(p4d= )); >>> } >>> >>> We should not run these tests for the above config because they are >>> not applicable and will invariably produce same result. >>> [...] >>>> >>>> So it shouldn't be an issue. Maybe if a couple of arches miss them, = the best would be to fix the arches, since that's the purpose of your tes= tsuite isn't it ? >>> >>> The run time failures as explained previously is because of the foldi= ng which >>> needs to be protected as they are not even applicable. The compile ti= me >>> failures are because pxx_populate() signatures are platform specific = depending >>> on how many page table levels they really support. >>> >> >> So IIUC, the compiletime problem is around __ARCH_HAS_5LEVEL_HACK. For= all #if !defined(__PAGETABLE_PXX_FOLDED), something equivalent to the fo= llowing should make the trick. >> >> =C2=A0=C2=A0=C2=A0=C2=A0if (mm_pxx_folded()) >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; >> >> >> For the __ARCH_HAS_5LEVEL_HACK stuff, I think we should be able to reg= roup all impacted functions inside a single #ifdef __ARCH_HAS_5LEVEL_HACK >=20 > I was wondering if it will be better to >=20 > 1) Minimize all #ifdefs in the code which might fail on some platforms > 2) Restrict proposed test module to platforms where it builds and runs > 3) Enable other platforms afterwards after fixing their build problems = or other requirements I understand that __ARCH_HAS_5LEVEL_HACK is an HACK as its name=20 suggests, so you can't expect all platforms to go for an HACK. I think=20 you can keep a single #ifdef __ARCH_HAS_5LEVEL_HACK / #else / #endif and=20 put all relevant tests inside it. For things like __PAGETABLE_PXX_FOLDED dependancies, I still think that=20 they can all be replaced by a runtime test of mm_pxx_folded(). Can you try that and see what problem remains ? >=20 > Would that be a better approach instead ? >=20 Based on the above, that might be the approach to take, yes. Christophe