From: Anshuman Khandual <anshuman.khandual@arm.com> To: Gerald Schaefer <gerald.schaefer@de.ibm.com> Cc: linux-mm@kvack.org, christophe.leroy@c-s.fr, Jonathan Corbet <corbet@lwn.net>, Andrew Morton <akpm@linux-foundation.org>, Mike Rapoport <rppt@linux.ibm.com>, Vineet Gupta <vgupta@synopsys.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Michael Ellerman <mpe@ellerman.id.au>, Heiko Carstens <heiko.carstens@de.ibm.com>, Vasily Gorbik <gor@linux.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, "H. Peter Anvin" <hpa@zytor.com>, "Kirill A . Shutemov" <kirill@shutemov.name>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, linux-snps-arc@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-riscv@lists.infradead.org, x86@kernel.org, linux-doc@vger.kernel.org, linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V2 0/3] mm/debug: Add more arch page table helper tests Date: Thu, 9 Apr 2020 06:36:23 +0530 Message-ID: <b35210ec-7084-9c77-bdf5-820cfc7f96bc@arm.com> (raw) In-Reply-To: <20200408141500.75b2e1a7@thinkpad> On 04/08/2020 05:45 PM, Gerald Schaefer wrote: > On Wed, 8 Apr 2020 12:41:51 +0530 > Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > [...] >>> >>>> >>>> Some thing like this instead. >>>> >>>> pte_t pte = READ_ONCE(*ptep); >>>> pte = pte_mkhuge(__pte((pte_val(pte) | RANDOM_ORVALUE) & PMD_MASK)); >>>> >>>> We cannot use mk_pte_phys() as it is defined only on some platforms >>>> without any generic fallback for others. >>> >>> Oh, didn't know that, sorry. What about using mk_pte() instead, at least >>> it would result in a present pte: >>> >>> pte = pte_mkhuge(mk_pte(phys_to_page(RANDOM_ORVALUE & PMD_MASK), prot)); >> >> Lets use mk_pte() here but can we do this instead >> >> paddr = (__pfn_to_phys(pfn) | RANDOM_ORVALUE) & PMD_MASK; >> pte = pte_mkhuge(mk_pte(phys_to_page(paddr), prot)); >> > > Sure, that will also work. > > BTW, this RANDOM_ORVALUE is not really very random, the way it is > defined. For s390 we already changed it to mask out some arch bits, > but I guess there are other archs and bits that would always be > set with this "not so random" value, and I wonder if/how that would > affect all the tests using this value, see also below. RANDOM_ORVALUE is a constant which was added in order to make sure that the page table entries should have some non-zero value before getting called with pxx_clear() and followed by a pxx_none() check. This is currently used only in pxx_clear_tests() tests. Hence there is no impact for the existing tests. > >>> >>> And if you also want to do some with the existing value, which seems >>> to be an empty pte, then maybe just check if writing and reading that >>> value with set_huge_pte_at() / huge_ptep_get() returns the same, >>> i.e. initially w/o RANDOM_ORVALUE. >>> >>> So, in combination, like this (BTW, why is the barrier() needed, it >>> is not used for the other set_huge_pte_at() calls later?): >> >> Ahh missed, will add them. Earlier we faced problem without it after >> set_pte_at() for a test on powerpc (64) platform. Hence just added it >> here to be extra careful. >> >>> >>> @@ -733,24 +733,28 @@ static void __init hugetlb_advanced_test >>> struct page *page = pfn_to_page(pfn); >>> pte_t pte = READ_ONCE(*ptep); >>> >>> - pte = __pte(pte_val(pte) | RANDOM_ORVALUE); >>> + set_huge_pte_at(mm, vaddr, ptep, pte); >>> + WARN_ON(!pte_same(pte, huge_ptep_get(ptep))); >>> + >>> + pte = pte_mkhuge(mk_pte(phys_to_page(RANDOM_ORVALUE & PMD_MASK), prot)); >>> set_huge_pte_at(mm, vaddr, ptep, pte); >>> barrier(); >>> WARN_ON(!pte_same(pte, huge_ptep_get(ptep))); >>> >>> This would actually add a new test "write empty pte with >>> set_huge_pte_at(), then verify with huge_ptep_get()", which happens >>> to trigger a warning on s390 :-) >> >> On arm64 as well which checks for pte_present() in set_huge_pte_at(). >> But PTE present check is not really present in each set_huge_pte_at() >> implementation especially without __HAVE_ARCH_HUGE_SET_HUGE_PTE_AT. >> Hence wondering if we should add this new test here which will keep >> giving warnings on s390 and arm64 (at the least). > > Hmm, interesting. I forgot about huge swap / migration, which is not > (and probably cannot be) supported on s390. The pte_present() check > on arm64 seems to check for such huge swap / migration entries, > according to the comment. > > The new test "write empty pte with set_huge_pte_at(), then verify > with huge_ptep_get()" would then probably trigger the > WARN_ON(!pte_present(pte)) in arm64 code. So I guess "writing empty > ptes with set_huge_pte_at()" is not really a valid use case in practice, > or else you would have seen this warning before. In that case, it > might not be a good idea to add this test. Got it. > > I also do wonder now, why the original test with > "pte = __pte(pte_val(pte) | RANDOM_ORVALUE);" > did not also trigger that warning on arm64. On s390 this test failed > exactly because the constructed pte was not present (initially empty, > or'ing RANDOM_ORVALUE does not make it present for s390). I guess this > just worked by chance on arm64, because the bits from RANDOM_ORVALUE > also happened to mark the pte present for arm64. That is correct. RANDOM_ORVALUE has got PTE_PROT_NONE bit set that makes the PTE test for pte_present(). On arm64 platform, #define pte_present(pte) (!!(pte_val(pte) & (PTE_VALID | PTE_PROT_NONE))) > > This brings us back to the question above, regarding the "randomness" > of RANDOM_ORVALUE. Not really sure what the intention behind that was, > but maybe it would make sense to restrict this RANDOM_ORVALUE to > non-arch-specific bits, i.e. only bits that would be part of the > address value within a page table entry? Or was it intentionally > chosen to also mess with other bits? As mentioned before, RANDOM_ORVALUE just helped make a given page table entry contain non-zero values before getting cleared. AFAICS we should not need RANDOM_ORVALUE for HugeTLB test here. I believe the following 'paddr' construct will just be fine instead. paddr = __pfn_to_phys(pfn) & PMD_MASK; pte = pte_mkhuge(mk_pte(phys_to_page(paddr), prot)); > > Regards, > Gerald > >
prev parent reply index Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-24 5:22 Anshuman Khandual 2020-03-24 5:22 ` [PATCH V2 1/3] mm/debug: Add tests validating arch page table helpers for core features Anshuman Khandual 2020-03-24 13:29 ` Zi Yan 2020-03-26 2:18 ` Anshuman Khandual 2020-03-26 17:08 ` Zi Yan 2020-03-30 8:56 ` [mm/debug] f675f2f91d: WARNING:at_mm/debug_vm_pgtable.c:#debug_vm_pgtable kernel test robot 2020-04-05 14:49 ` Anshuman Khandual 2020-03-24 5:22 ` [PATCH V2 2/3] mm/debug: Add tests validating arch advanced page table helpers Anshuman Khandual 2020-03-27 7:41 ` [mm/debug] d157503f6f: WARNING:at_mm/debug_vm_pgtable.c:#debug_vm_pgtable kernel test robot 2020-03-24 5:22 ` [PATCH V2 3/3] Documentation/mm: Add descriptions for arch page table helpers Anshuman Khandual 2020-03-26 2:23 ` [PATCH V2 0/3] mm/debug: Add more arch page table helper tests Anshuman Khandual 2020-03-26 15:23 ` Christophe Leroy 2020-03-27 6:46 ` Anshuman Khandual 2020-03-27 7:00 ` Christophe Leroy 2020-03-29 14:21 ` Anshuman Khandual 2020-03-31 12:30 ` Gerald Schaefer 2020-04-05 12:28 ` Anshuman Khandual 2020-04-07 15:54 ` Gerald Schaefer 2020-04-08 7:11 ` Anshuman Khandual 2020-04-08 12:15 ` Gerald Schaefer 2020-04-09 1:06 ` Anshuman Khandual [this message]
Reply instructions: You may reply publicly 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=b35210ec-7084-9c77-bdf5-820cfc7f96bc@arm.com \ --to=anshuman.khandual@arm.com \ --cc=akpm@linux-foundation.org \ --cc=benh@kernel.crashing.org \ --cc=borntraeger@de.ibm.com \ --cc=bp@alien8.de \ --cc=catalin.marinas@arm.com \ --cc=christophe.leroy@c-s.fr \ --cc=corbet@lwn.net \ --cc=gerald.schaefer@de.ibm.com \ --cc=gor@linux.ibm.com \ --cc=heiko.carstens@de.ibm.com \ --cc=hpa@zytor.com \ --cc=kirill@shutemov.name \ --cc=linux-arch@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linux-riscv@lists.infradead.org \ --cc=linux-s390@vger.kernel.org \ --cc=linux-snps-arc@lists.infradead.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mingo@redhat.com \ --cc=mpe@ellerman.id.au \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=paulus@samba.org \ --cc=rppt@linux.ibm.com \ --cc=tglx@linutronix.de \ --cc=vgupta@synopsys.com \ --cc=will@kernel.org \ --cc=x86@kernel.org \ /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
LKML Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lkml.kernel.org/lkml/0 lkml/git/0.git git clone --mirror https://lkml.kernel.org/lkml/1 lkml/git/1.git git clone --mirror https://lkml.kernel.org/lkml/2 lkml/git/2.git git clone --mirror https://lkml.kernel.org/lkml/3 lkml/git/3.git git clone --mirror https://lkml.kernel.org/lkml/4 lkml/git/4.git git clone --mirror https://lkml.kernel.org/lkml/5 lkml/git/5.git git clone --mirror https://lkml.kernel.org/lkml/6 lkml/git/6.git git clone --mirror https://lkml.kernel.org/lkml/7 lkml/git/7.git git clone --mirror https://lkml.kernel.org/lkml/8 lkml/git/8.git git clone --mirror https://lkml.kernel.org/lkml/9 lkml/git/9.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 lkml lkml/ https://lkml.kernel.org/lkml \ linux-kernel@vger.kernel.org public-inbox-index lkml Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git