Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
From: Christophe Leroy <christophe.leroy@c-s.fr>
To: Anshuman Khandual <anshuman.khandual@arm.com>, linux-mm@kvack.org
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Vlastimil Babka <vbabka@suse.cz>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	Dan Williams <dan.j.williams@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Michal Hocko <mhocko@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Mark Brown <broonie@kernel.org>,
	Steven Price <Steven.Price@arm.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Kees Cook <keescook@chromium.org>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	Matthew Wilcox <willy@infradead.org>,
	Sri Krishna chowdary <schowdary@nvidia.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Russell King - ARM Linux <linux@armlinux.org.uk>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Paul Mackerras <paulus@samba.org>,
	Martin Schwidefsky <schwidefsky@de.ibm.com>,
	Heiko Carstens <heiko.carstens@de.ibm.com>,
	"David S. Miller" <davem@davemloft.net>,
	Vineet Gupta <vgupta@synopsys.com>,
	James Hogan <jhogan@kernel.org>,
	Paul Burton <paul.burton@mips.com>,
	Ralf Baechle <ralf@linux-mips.org>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	Gerald Schaefer <gerald.schaefer@de.ibm.com>,
	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
Subject: Re: [PATCH V2 2/2] mm/pgtable/debug: Add test validating architecture page table helpers
Date: Wed, 18 Sep 2019 18:26:03 +0200
Message-ID: <64504101-d9dd-f273-02f9-e9a8b178eecc@c-s.fr> (raw)
In-Reply-To: <f872e6f4-a5cb-069d-2034-78961930cb9f@arm.com>



Le 18/09/2019 à 07:04, Anshuman Khandual a écrit :
> 
> 
> On 09/13/2019 03:31 PM, Christophe Leroy wrote:
>>
>>
>> Le 13/09/2019 à 11:02, Anshuman Khandual a écrit :
>>>
>>>>> +#if !defined(__PAGETABLE_PMD_FOLDED) && !defined(__ARCH_HAS_4LEVEL_HACK)
>>>>
>>>> #ifdefs have to be avoided as much as possible, see below
>>>
>>> Yeah but it has been bit difficult to avoid all these $ifdef because of the
>>> availability (or lack of it) for all these pgtable helpers in various 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 around 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)
> {
>          p4d_t p4d = 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 
top of the function:

	if (mm_pud_folded(mm) || is_defined(__ARCH_HAS_5LEVEL_HACK))
		return;

> 
>          p4d = __p4d(p4d_val(p4d) | RANDOM_ORVALUE);
>          WRITE_ONCE(*p4dp, p4d);
>          p4d_clear(p4dp);
>          p4d = READ_ONCE(*p4dp);
>          WARN_ON(!p4d_none(p4d));
> }
> 
> The following test hits an error at WARN_ON(!p4d_none(p4d))
> 
> [   16.757333] ------------[ cut here ]------------
> [   16.758019] WARNING: CPU: 11 PID: 1 at mm/arch_pgtable_test.c:187 arch_pgtable_tests_init+0x24c/0x474
> [   16.759455] Modules linked in:
> [   16.759952] CPU: 11 PID: 1 Comm: swapper/0 Not tainted 5.3.0-next-20190916-00005-g61c218153bb8-dirty #222
> [   16.761449] Hardware name: linux,dummy-virt (DT)
> [   16.762185] pstate: 00400005 (nzcv daif +PAN -UAO)
> [   16.762964] pc : arch_pgtable_tests_init+0x24c/0x474
> [   16.763750] lr : arch_pgtable_tests_init+0x174/0x474
> [   16.764534] sp : ffffffc011d7bd50
> [   16.765065] x29: ffffffc011d7bd50 x28: ffffffff1756bac0
> [   16.765908] x27: ffffff85ddaf3000 x26: 00000000000002e8
> [   16.766767] x25: ffffffc0111ce000 x24: ffffff85ddaf32e8
> [   16.767606] x23: ffffff85ddaef278 x22: 00000045cc844000
> [   16.768445] x21: 000000065daef003 x20: ffffffff17540000
> [   16.769283] x19: ffffff85ddb60000 x18: 0000000000000014
> [   16.770122] x17: 00000000980426bb x16: 00000000698594c6
> [   16.770976] x15: 0000000066e25a88 x14: 0000000000000000
> [   16.771813] x13: ffffffff17540000 x12: 000000000000000a
> [   16.772651] x11: ffffff85fcfd0a40 x10: 0000000000000001
> [   16.773488] x9 : 0000000000000008 x8 : ffffffc01143ab26
> [   16.774336] x7 : 0000000000000000 x6 : 0000000000000000
> [   16.775180] x5 : 0000000000000000 x4 : 0000000000000000
> [   16.776018] x3 : ffffffff1756bbe8 x2 : 000000065daeb003
> [   16.776856] x1 : 000000000065daeb x0 : fffffffffffff000
> [   16.777693] Call trace:
> [   16.778092]  arch_pgtable_tests_init+0x24c/0x474
> [   16.778843]  do_one_initcall+0x74/0x1b0
> [   16.779458]  kernel_init_freeable+0x1cc/0x290
> [   16.780151]  kernel_init+0x10/0x100
> [   16.780710]  ret_from_fork+0x10/0x18
> [   16.781282] ---[ end trace 042e6c40c0a3b038 ]---
> 
> On arm64 (4K page size|39 bits VA|3 level page table)
> 
> #elif CONFIG_PGTABLE_LEVELS == 3	/* Applicable here */
> #define __ARCH_USE_5LEVEL_HACK
> #include <asm-generic/pgtable-nopud.h>
> 
> Which pulls in
> 
> #include <asm-generic/pgtable-nop4d-hack.h>
> 
> which pulls in
> 
> #include <asm-generic/5level-fixup.h>
> 
> which defines
> 
> static inline int p4d_none(p4d_t p4d)
> {
>          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)
> {
>          return 0;
> }
> 
> static void __init p4d_populate_tests(struct mm_struct *mm, p4d_t *p4dp,
>                                        pud_t *pudp)
> {
>          p4d_t p4d;
> 
>          /*
>           * This entry points to next level page table page.
>           * Hence this must not qualify as p4d_bad().
>           */
>          pud_clear(pudp);
>          p4d_clear(p4dp);
>          p4d_populate(mm, p4dp, pudp);
>          p4d = READ_ONCE(*p4dp);
>          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.
> 
>>
>>>
>>>>
>>
>> [...]
>>
>>>>> +#if !defined(__PAGETABLE_PUD_FOLDED) && !defined(__ARCH_HAS_5LEVEL_HACK)
>>>>
>>>> The same can be done here.
>>>
>>> IIRC not only the page table helpers but there are data types (pxx_t) which
>>> were not present on various configs and these wrappers help prevent build
>>> failures. Any ways will try and see if this can be improved further. But
>>> meanwhile if you have some suggestions, please do let me know.
>>
>> pgt_t and pmd_t are everywhere I guess.
>> then pud_t and p4d_t have fallbacks in asm-generic files.
> 
> Lets take another example where it fails to compile. On arm64 with 16K
> page size, 48 bits VA, 4 level page table arrangement in the following
> test, pgd_populate() does not have the required signature.
> 
> static void pgd_populate_tests(struct mm_struct *mm, pgd_t *pgdp, p4d_t *p4dp)
> {
>          pgd_t pgd;
> 
>          if (mm_p4d_folded(mm))
>                  return;
> 
>         /*
>           * This entry points to next level page table page.
>           * Hence this must not qualify as pgd_bad().
>           */
>          p4d_clear(p4dp);
>          pgd_clear(pgdp);
>          pgd_populate(mm, pgdp, p4dp);
>          pgd = READ_ONCE(*pgdp);
>          WARN_ON(pgd_bad(pgd));
> }
> 
> mm/arch_pgtable_test.c: In function ‘pgd_populate_tests’:
> mm/arch_pgtable_test.c:254:25: error: passing argument 3 of ‘pgd_populate’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>    pgd_populate(mm, pgdp, p4dp);
>                           ^~~~
> In file included from mm/arch_pgtable_test.c:27:0:
> ./arch/arm64/include/asm/pgalloc.h:81:20: note: expected ‘pud_t * {aka struct <anonymous> *}’ but argument is of type ‘pgd_t * {aka struct <anonymous> *}’
>   static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgdp, pud_t *pudp)
> 
> The build failure is because p4d_t * maps to pgd_t * but the applicable
> (it does not fallback on generic ones) pgd_populate() expects a pud_t *.
> 
> Except for archs which have 5 level page able, pgd_populate() always accepts
> lower level page table pointers as the last argument as they dont have that
> many levels.
> 
> arch/x86/include/asm/pgalloc.h:static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgd, p4d_t *p4d)
> arch/s390/include/asm/pgalloc.h:static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgd, p4d_t *p4d)
> 
> But others
> 
> arch/arm64/include/asm/pgalloc.h:static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgdp, pud_t *pudp)
> arch/m68k/include/asm/motorola_pgalloc.h:static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgd, pmd_t *pmd)
> arch/mips/include/asm/pgalloc.h:static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgd, pud_t *pud)
> arch/powerpc/include/asm/book3s/64/pgalloc.h:static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgd, pud_t *pud)
> 
> I remember going through all these combinations before arriving at the
> current state of #ifdef exclusions. Probably, to solved this all platforms
> have to define pxx_populate() helpers assuming they support 5 level page
> table.
> 
>>
>> 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 testsuite isn't it ?
> 
> The run time failures as explained previously is because of the folding which
> needs to be protected as they are not even applicable. The compile time
> 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 
following should make the trick.

	if (mm_pxx_folded())
		return;


For the __ARCH_HAS_5LEVEL_HACK stuff, I think we should be able to 
regroup all impacted functions inside a single #ifdef 
__ARCH_HAS_5LEVEL_HACK

Christophe

  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12  6:02 [PATCH V2 0/2] mm/debug: Add tests for architecture exported " Anshuman Khandual
2019-09-12  6:02 ` [PATCH V2 2/2] mm/pgtable/debug: Add test validating architecture " Anshuman Khandual
2019-09-12 11:00   ` Kirill A. Shutemov
2019-09-12 12:09     ` Anshuman Khandual
2019-09-12 15:00   ` Christophe Leroy
2019-09-12 15:36     ` Christophe Leroy
2019-09-12 15:52       ` Christophe Leroy
2019-09-13  6:30         ` Christophe Leroy
2019-09-12 17:14   ` Christophe Leroy
2019-09-13  6:23     ` [PATCH] mm/pgtable/debug: Fix " Christophe Leroy
2019-09-13  6:58       ` Anshuman Khandual
2019-09-13  7:03         ` Christophe Leroy
2019-09-13  7:11           ` Christophe Leroy
2019-09-13  8:42             ` Anshuman Khandual
2019-09-13  8:51               ` Kirill A. Shutemov
2019-09-18  7:32       ` Anshuman Khandual
2019-09-19  5:44         ` Christophe Leroy
2019-09-13  9:02     ` [PATCH V2 2/2] mm/pgtable/debug: Add " Anshuman Khandual
2019-09-13  9:13       ` Kirill A. Shutemov
2019-09-13 10:01       ` Christophe Leroy
2019-09-18  5:04         ` Anshuman Khandual
2019-09-18 16:26           ` Christophe Leroy [this message]
2019-09-18 18:22             ` Gerald Schaefer
2019-09-20  4:06               ` Anshuman Khandual
2019-09-19  4:56             ` Anshuman Khandual
2019-09-19  5:41               ` Christophe Leroy
2019-09-12 14:42 ` [PATCH V2 0/2] mm/debug: Add tests for architecture exported " Christophe Leroy
2019-09-13  6:24   ` Anshuman Khandual
2019-09-13  6:32     ` Christophe Leroy

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=64504101-d9dd-f273-02f9-e9a8b178eecc@c-s.fr \
    --to=christophe.leroy@c-s.fr \
    --cc=Steven.Price@arm.com \
    --cc=akpm@linux-foundation.org \
    --cc=anshuman.khandual@arm.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=broonie@kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=davem@davemloft.net \
    --cc=gerald.schaefer@de.ibm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heiko.carstens@de.ibm.com \
    --cc=jgg@ziepe.ca \
    --cc=jhogan@kernel.org \
    --cc=keescook@chromium.org \
    --cc=kirill@shutemov.name \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-ia64@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linux-sh@vger.kernel.org \
    --cc=linux-snps-arc@lists.infradead.org \
    --cc=linux@armlinux.org.uk \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mark.rutland@arm.com \
    --cc=mhocko@kernel.org \
    --cc=mpe@ellerman.id.au \
    --cc=paul.burton@mips.com \
    --cc=paulus@samba.org \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=peterz@infradead.org \
    --cc=ralf@linux-mips.org \
    --cc=rppt@linux.vnet.ibm.com \
    --cc=schowdary@nvidia.com \
    --cc=schwidefsky@de.ibm.com \
    --cc=sparclinux@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=vbabka@suse.cz \
    --cc=vgupta@synopsys.com \
    --cc=willy@infradead.org \
    --cc=x86@kernel.org \
    --cc=yamada.masahiro@socionext.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git