From: Hugh Dickins <hughd@google.com>
To: Nathan Chancellor <nathan@kernel.org>
Cc: Hugh Dickins <hughd@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
Mike Kravetz <mike.kravetz@oracle.com>,
Mike Rapoport <rppt@kernel.org>,
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
Matthew Wilcox <willy@infradead.org>,
David Hildenbrand <david@redhat.com>,
Suren Baghdasaryan <surenb@google.com>,
Qi Zheng <zhengqi.arch@bytedance.com>,
Peter Zijlstra <peterz@infradead.org>,
Russell King <linux@armlinux.org.uk>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
Geert Uytterhoeven <geert@linux-m68k.org>,
Greg Ungerer <gerg@linux-m68k.org>,
Michal Simek <monstr@monstr.eu>,
Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
Helge Deller <deller@gmx.de>,
John David Anglin <dave.anglin@bell.net>,
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
Michael Ellerman <mpe@ellerman.id.au>,
Alexandre Ghiti <alexghiti@rivosinc.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Heiko Carstens <hca@linux.ibm.com>,
Christian Borntraeger <borntraeger@linux.ibm.com>,
Claudio Imbrenda <imbrenda@linux.ibm.com>,
Alexander Gordeev <agordeev@linux.ibm.com>,
John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>,
"David S. Miller" <davem@davemloft.net>,
Chris Zankel <chris@zankel.net>,
Max Filippov <jcmvbkbc@gmail.com>,
x86@kernel.org, linux-arm-kernel@lists.infradead.org,
linux-ia64@vger.kernel.org, linux-m68k@lists.linux-m68k.org,
linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-riscv@lists.infradead.org,
linux-s390@vger.kernel.org, linux-sh@vger.kernel.org,
sparclinux@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org
Subject: Re: [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb()
Date: Wed, 14 Jun 2023 22:43:30 -0700 (PDT) [thread overview]
Message-ID: <344a4da-3890-45fd-607e-b5f85ca6ad48@google.com> (raw)
In-Reply-To: <f5526f17-9d78-f7ea-427a-7e76bfeb6b8@google.com>
[-- Attachment #1: Type: text/plain, Size: 3563 bytes --]
On Wed, 14 Jun 2023, Hugh Dickins wrote:
> On Wed, 14 Jun 2023, Nathan Chancellor wrote:
> >
> > I just bisected a crash while powering down a MIPS machine in QEMU to
> > this change as commit 8044511d3893 ("mips: update_mmu_cache() can
> > replace __update_tlb()") in linux-next.
>
> Thank you, Nathan, that's very helpful indeed. This patch certainly knew
> that it wanted testing, and I'm glad to hear that it is now seeing some.
>
> While powering down? The messages below look like it was just coming up,
> but no doubt that's because you were bisecting (or because I'm unfamiliar
> with what messages to expect there). It's probably irrelevant information,
> but I wonder whether the (V)machine worked well enough for a while before
> you first powered down and spotted the problem, or whether it's never got
> much further than trying to run init (busybox)? I'm trying to get a feel
> for whether the problem occurs under common or uncommon conditions.
>
> > Unfortunately, I can still
> > reproduce it with the existing fix you have for this change on the
> > mailing list, which is present in next-20230614.
>
> Right, that later fix was only for a build warning, nothing functional
> (or at least I hoped that it wasn't making any functional difference).
>
> Thanks a lot for the detailed instructions below: unfortunately, those
> would draw me into a realm of testing I've never needed to enter before,
> so a lot of time spent on setup and learning. Usually, I just stare at
> the source.
>
> What this probably says is that I should revert most my cleanup there,
> and keep as close to the existing code as possible. But some change is
> needed, and I may need to understand (or have a good guess at) what was
> going wrong, to decide what kind of retreat will be successful.
>
> Back to the source for a while: I hope I'll find examples in nearby MIPS
> kernel source (and git history), which will hint at the right way forward.
> Then send you a patch against next-20230614 to try, when I'm reasonably
> confident that it's enough to satisfy my purpose, but likely not to waste
> your time.
I'm going to take advantage of your good nature by attaching
two alternative patches, either to go on top of next-20230614.
mips1.patch,
arch/mips/mm/tlb-r4k.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
is by far my favourite. I couldn't see anything wrong with what's
already there for mips, but it seems possible that (though I didn't
find it) somewhere calls update_mmu_cache_pmd() on a page table. So
mips1.patch restores the pmd_huge() check, and cleans up further by
removing the silly pgdp, p4dp, pudp, pmdp stuff: the pointer has now
been passed in by the caller, why walk the tree again? I should have
done it this way before.
But if that doesn't work, then I'm afraid it will have to be
mips2.patch,
arch/mips/include/asm/pgtable.h | 15 ++++++++++++---
arch/mips/mm/tlb-r3k.c | 5 ++---
arch/mips/mm/tlb-r4k.c | 27 ++++++++++++++++++---------
3 files changed, 32 insertions(+), 15 deletions(-)
which reverts all of the original patch and its build warning fix,
and does a pte_unmap() to balance the silly pte_offset_map() there;
with an apologetic comment for this being about the only place in
the tree where I have no idea what to do if ptep were NULL.
I do hope that you find the first fixes the breakage; but if not, then
I even more fervently hope that the second will, despite my hating it.
Touch wood for the first, fingers crossed for the second, thanks,
Hugh
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch; name=mips1.patch, Size: 900 bytes --]
--- a/arch/mips/mm/tlb-r4k.c
+++ b/arch/mips/mm/tlb-r4k.c
@@ -293,12 +293,6 @@ void local_flush_tlb_one(unsigned long page)
void update_mmu_cache(struct vm_area_struct *vma,
unsigned long address, pte_t *ptep)
{
-#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
- pgd_t *pgdp;
- p4d_t *p4dp;
- pud_t *pudp;
- pmd_t *pmdp;
-#endif
unsigned long flags;
int idx, pid;
@@ -323,12 +317,8 @@ void update_mmu_cache(struct vm_area_struct *vma,
tlb_probe_hazard();
idx = read_c0_index();
#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
- pgdp = pgd_offset(vma->vm_mm, address);
- p4dp = p4d_offset(pgdp, address);
- pudp = pud_offset(p4dp, address);
- pmdp = pmd_offset(pudp, address);
/* this could be a huge page */
- if (ptep == (pte_t *)pmdp) {
+ if (pmd_huge(*(pmd_t *)ptep)) {
unsigned long lo;
write_c0_pagemask(PM_HUGE_MASK);
lo = pte_to_entrylo(pte_val(*ptep));
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-patch; name=mips2.patch, Size: 3927 bytes --]
--- a/arch/mips/include/asm/pgtable.h
+++ b/arch/mips/include/asm/pgtable.h
@@ -565,8 +565,15 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
}
#endif
-extern void update_mmu_cache(struct vm_area_struct *vma,
- unsigned long address, pte_t *ptep);
+extern void __update_tlb(struct vm_area_struct *vma, unsigned long address,
+ pte_t pte);
+
+static inline void update_mmu_cache(struct vm_area_struct *vma,
+ unsigned long address, pte_t *ptep)
+{
+ pte_t pte = *ptep;
+ __update_tlb(vma, address, pte);
+}
#define __HAVE_ARCH_UPDATE_MMU_TLB
#define update_mmu_tlb update_mmu_cache
@@ -574,7 +581,9 @@ extern void update_mmu_cache(struct vm_area_struct *vma,
static inline void update_mmu_cache_pmd(struct vm_area_struct *vma,
unsigned long address, pmd_t *pmdp)
{
- update_mmu_cache(vma, address, (pte_t *)pmdp);
+ pte_t pte = *(pte_t *)pmdp;
+
+ __update_tlb(vma, address, pte);
}
/*
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -176,8 +176,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
}
}
-void update_mmu_cache(struct vm_area_struct *vma,
- unsigned long address, pte_t *ptep)
+void __update_tlb(struct vm_area_struct *vma, unsigned long address, pte_t pte)
{
unsigned long asid_mask = cpu_asid_mask(¤t_cpu_data);
unsigned long flags;
@@ -204,7 +203,7 @@ void update_mmu_cache(struct vm_area_struct *vma,
BARRIER;
tlb_probe();
idx = read_c0_index();
- write_c0_entrylo0(pte_val(*ptep));
+ write_c0_entrylo0(pte_val(pte));
write_c0_entryhi(address | pid);
if (idx < 0) { /* BARRIER */
tlb_write_random();
--- a/arch/mips/mm/tlb-r4k.c
+++ b/arch/mips/mm/tlb-r4k.c
@@ -290,16 +290,14 @@ void local_flush_tlb_one(unsigned long page)
* updates the TLB with the new pte(s), and another which also checks
* for the R4k "end of page" hardware bug and does the needy.
*/
-void update_mmu_cache(struct vm_area_struct *vma,
- unsigned long address, pte_t *ptep)
+void __update_tlb(struct vm_area_struct * vma, unsigned long address, pte_t pte)
{
-#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
+ unsigned long flags;
pgd_t *pgdp;
p4d_t *p4dp;
pud_t *pudp;
pmd_t *pmdp;
-#endif
- unsigned long flags;
+ pte_t *ptep, *ptemap = NULL;
int idx, pid;
/*
@@ -318,19 +316,20 @@ void update_mmu_cache(struct vm_area_struct *vma,
pid = read_c0_entryhi() & cpu_asid_mask(¤t_cpu_data);
write_c0_entryhi(address | pid);
}
+ pgdp = pgd_offset(vma->vm_mm, address);
mtc0_tlbw_hazard();
tlb_probe();
tlb_probe_hazard();
- idx = read_c0_index();
-#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
- pgdp = pgd_offset(vma->vm_mm, address);
p4dp = p4d_offset(pgdp, address);
pudp = pud_offset(p4dp, address);
pmdp = pmd_offset(pudp, address);
+ idx = read_c0_index();
+#ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
/* this could be a huge page */
- if (ptep == (pte_t *)pmdp) {
+ if (pmd_huge(*pmdp)) {
unsigned long lo;
write_c0_pagemask(PM_HUGE_MASK);
+ ptep = (pte_t *)pmdp;
lo = pte_to_entrylo(pte_val(*ptep));
write_c0_entrylo0(lo);
write_c0_entrylo1(lo + (HPAGE_SIZE >> 7));
@@ -345,6 +344,13 @@ void update_mmu_cache(struct vm_area_struct *vma,
} else
#endif
{
+ ptemap = ptep = pte_offset_map(pmdp, address);
+ /*
+ * update_mmu_cache() is called between pte_offset_map_lock()
+ * and pte_unmap_unlock(), so we can assume that ptep is not
+ * NULL here: and what should be done below if it were NULL?
+ */
+
#if defined(CONFIG_PHYS_ADDR_T_64BIT) && defined(CONFIG_CPU_MIPS32)
#ifdef CONFIG_XPA
write_c0_entrylo0(pte_to_entrylo(ptep->pte_high));
@@ -372,6 +378,9 @@ void update_mmu_cache(struct vm_area_struct *vma,
tlbw_use_hazard();
htw_start();
flush_micro_tlb_vm(vma);
+
+ if (ptemap)
+ pte_unmap(ptemap);
local_irq_restore(flags);
}
[-- Attachment #4: Type: text/plain, Size: 161 bytes --]
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2023-06-15 5:43 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-08 19:07 [PATCH v2 00/23] arch: allow pte_offset_map[_lock]() to fail Hugh Dickins
2023-06-08 19:10 ` [PATCH v2 01/23] arm: " Hugh Dickins
2023-06-08 19:11 ` [PATCH v2 02/23] arm64: allow pte_offset_map() " Hugh Dickins
2023-06-08 19:13 ` [PATCH v2 03/23] arm64/hugetlb: pte_alloc_huge() pte_offset_huge() Hugh Dickins
2023-06-08 19:14 ` [PATCH v2 04/23] ia64/hugetlb: " Hugh Dickins
2023-06-08 19:15 ` [PATCH v2 05/23] m68k: allow pte_offset_map[_lock]() to fail Hugh Dickins
2023-06-08 19:16 ` [PATCH v2 06/23] microblaze: allow pte_offset_map() " Hugh Dickins
2023-06-08 19:17 ` [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb() Hugh Dickins
2023-06-09 8:08 ` [PATCH v2 07/23 fix] mips: update_mmu_cache() can replace __update_tlb(): fix Hugh Dickins
2023-06-14 23:17 ` [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb() Nathan Chancellor
2023-06-15 0:26 ` Hugh Dickins
2023-06-15 5:43 ` Hugh Dickins [this message]
2023-06-15 15:50 ` Nathan Chancellor
2023-06-15 21:22 ` Hugh Dickins
2023-06-15 23:02 ` [PATCH v2 07/23 replacement] mips: add pte_unmap() to balance pte_offset_map() Hugh Dickins
2023-06-17 3:54 ` Yu Zhao
2023-06-18 20:57 ` Yu Zhao
2023-06-15 22:07 ` [PATCH v2 07/23] mips: update_mmu_cache() can replace __update_tlb() Yu Zhao
2023-06-08 19:18 ` [PATCH v2 08/23] parisc: add pte_unmap() to balance get_ptep() Hugh Dickins
2023-06-19 3:55 ` Helge Deller
2023-06-08 19:20 ` [PATCH v2 09/23] parisc: unmap_uncached_pte() use pte_offset_kernel() Hugh Dickins
2023-06-08 19:21 ` [PATCH v2 10/23] parisc/hugetlb: pte_alloc_huge() pte_offset_huge() Hugh Dickins
2023-06-08 19:22 ` [PATCH v2 11/23] powerpc: kvmppc_unmap_free_pmd() pte_offset_kernel() Hugh Dickins
2023-06-08 19:23 ` [PATCH v2 12/23] powerpc: allow pte_offset_map[_lock]() to fail Hugh Dickins
2023-06-08 19:24 ` [PATCH v2 13/23] powerpc/hugetlb: pte_alloc_huge() Hugh Dickins
2023-06-08 19:25 ` [PATCH v2 14/23] riscv/hugetlb: pte_alloc_huge() pte_offset_huge() Hugh Dickins
2023-06-08 19:27 ` [PATCH v2 15/23] s390: allow pte_offset_map_lock() to fail Hugh Dickins
2023-06-13 11:45 ` Claudio Imbrenda
2023-06-08 19:29 ` [PATCH v2 16/23] s390: gmap use pte_unmap_unlock() not spin_unlock() Hugh Dickins
2023-06-08 19:30 ` [PATCH v2 17/23] sh/hugetlb: pte_alloc_huge() pte_offset_huge() Hugh Dickins
2023-06-08 19:31 ` [PATCH v2 18/23] sparc/hugetlb: " Hugh Dickins
2023-06-08 19:32 ` [PATCH v2 19/23] sparc: allow pte_offset_map() to fail Hugh Dickins
2023-06-08 19:33 ` [PATCH v2 20/23] sparc: iounit and iommu use pte_offset_kernel() Hugh Dickins
2023-06-08 19:35 ` [PATCH v2 21/23] x86: Allow get_locked_pte() to fail Hugh Dickins
2023-06-08 19:36 ` [PATCH v2 22/23] x86: sme_populate_pgd() use pte_offset_kernel() Hugh Dickins
2023-06-08 19:37 ` [PATCH v2 23/23] xtensa: add pte_unmap() to balance pte_offset_map() Hugh Dickins
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=344a4da-3890-45fd-607e-b5f85ca6ad48@google.com \
--to=hughd@google.com \
--cc=agordeev@linux.ibm.com \
--cc=akpm@linux-foundation.org \
--cc=alexghiti@rivosinc.com \
--cc=aneesh.kumar@linux.ibm.com \
--cc=borntraeger@linux.ibm.com \
--cc=catalin.marinas@arm.com \
--cc=chris@zankel.net \
--cc=dave.anglin@bell.net \
--cc=davem@davemloft.net \
--cc=david@redhat.com \
--cc=deller@gmx.de \
--cc=geert@linux-m68k.org \
--cc=gerg@linux-m68k.org \
--cc=glaubitz@physik.fu-berlin.de \
--cc=hca@linux.ibm.com \
--cc=imbrenda@linux.ibm.com \
--cc=jcmvbkbc@gmail.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-ia64@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-m68k@lists.linux-m68k.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-parisc@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-sh@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mike.kravetz@oracle.com \
--cc=monstr@monstr.eu \
--cc=mpe@ellerman.id.au \
--cc=nathan@kernel.org \
--cc=palmer@dabbelt.com \
--cc=peterz@infradead.org \
--cc=rppt@kernel.org \
--cc=sparclinux@vger.kernel.org \
--cc=surenb@google.com \
--cc=tsbogend@alpha.franken.de \
--cc=will@kernel.org \
--cc=willy@infradead.org \
--cc=x86@kernel.org \
--cc=zhengqi.arch@bytedance.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).