From: Ira Weiny <ira.weiny@intel.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-kernel@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
Christian Koenig <christian.koenig@amd.com>,
Huang Rui <ray.huang@amd.com>,
Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
"James E.J. Bottomley" <James.Bottomley@hansenpartnership.com>,
Helge Deller <deller@gmx.de>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
"David S. Miller" <davem@davemloft.net>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
Dave Hansen <dave.hansen@linux.intel.com>,
Andy Lutomirski <luto@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Chris Zankel <chris@zankel.net>,
Max Filippov <jcmvbkbc@gmail.com>,
Dan Williams <dan.j.williams@intel.com>,
linux-snps-arc@lists.infradead.org,
linux-arm-kernel@lists.infradead.org, linux-csky@vger.kernel.org,
linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, sparclinux@vger.kernel.org,
linux-xtensa@linux-xtensa.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH V2 00/11] Subject: Remove duplicated kmap code
Date: Mon, 4 May 2020 13:17:41 -0700 [thread overview]
Message-ID: <20200504201740.GA985739@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20200504053357.GV23230@ZenIV.linux.org.uk>
On Mon, May 04, 2020 at 06:33:57AM +0100, Al Viro wrote:
> On Sun, May 03, 2020 at 10:04:47PM -0700, Ira Weiny wrote:
>
> > Grepping for 'asm/highmem.h' and investigations don't reveal any issues... But
> > you do have me worried. That said 0-day has been crunching on multiple
> > versions of this series without issues such as this (save the mips issue
> > above).
> >
> > I have to say it would be nice if the relation between linux/highmem.h and
> > asm/highmem.h was more straightforward.
>
> IIRC, the headache was due to asm/pgtable.h on several architectures and
> asm/cacheflush.h on parisc.
>
> <digs the notes out>
>
> || IOW, there's one in linux/highmem.h (conditional on !CONFIG_HIGHMEM,
> || !ARCH_HAS_KMAP) and several per-architecture variants, usually declared in
> || their asm/highmem.h. In three of those (microblaze, parisc and powerpc) these
> || are inlines (parisc one identical to linux/highmem.h, lives in asm/cacheflush.h,
> || powerpc and microblaze ones calling kmap_atomic_prot() which is defined in
> || arch/$ARCH/mm/highmem.c).
> ||
> || parisc case is weird - essentially, they want to call
> || flush_kernel_dcache_page_addr() at the places where kunmap/kunmap_atomic
> || is done. And they do so despite not selecting HIGHMEM, with definitions
> || in usual place. They do have ARCH_HAS_KMAP defined, which prevents
> || getting buggered in linux/highmem.h. ARCH_HAS_KMAP is parisc-unique,
> || BTW, and checked only in linux/highmem.h.
> ||
> || All genuine arch-specific variants are defined in (or call functions
> || defined in) translation units that are only included CONFIG_HIGHMEM builds.
I agree with this statement. But IMO additional confusion is caused by the
fact that some arch's condition the declarations on CONFIG_HIGHMEM within
asm/highmem.h (arc, arm, nds32) while others depend on linux/highmem.h (and
elsewhere) to do so (csky, microblaze, mips, powerpc, sparc, x86, xtensa).
Why?
I think (perhaps naive) over time asm/highmem.h needs to be isolated to being
included in linux/highmem.h. But as you point out below that is not so easy.
I think that this series helps toward that goal.
> ||
> || It would be tempting to consolidate those, e.g. by adding __kmap_atomic()
> || and __kmap_atomic_prot() without that boilerplate, with universal kmap_atomic()
> || and kmap_atomic_prot() in linux/highmem.h. Potential problem with that would
> || be something that pulls ash/highmem.h (or asm/cacheflush.h in case of parisc)
> || directly and uses kmap_atomic/kmap_atomic_prot. There's not a lot places
> || pulling asm/highmem.h, and many of them are not even in includes:
> ||
> || arch/arm/include/asm/efi.h:13:#include <asm/highmem.h>
> || arch/arm/mm/dma-mapping.c:31:#include <asm/highmem.h>
> || arch/arm/mm/flush.c:14:#include <asm/highmem.h>
> || arch/arm/mm/mmu.c:27:#include <asm/highmem.h>
> || arch/mips/include/asm/pgtable-32.h:22:#include <asm/highmem.h>
> || arch/mips/mm/cache.c:19:#include <asm/highmem.h>
> || arch/mips/mm/fault.c:28:#include <asm/highmem.h> /* For VMALLOC_END */
> || arch/nds32/include/asm/pgtable.h:60:#include <asm/highmem.h>
> || arch/x86/kernel/setup_percpu.c:20:#include <asm/highmem.h>
> || include/linux/highmem.h:35:#include <asm/highmem.h>
> ||
> || Users of asm/cacheflush.h are rather more numerous; however, anything
> || outside of parisc-specific code has to pull linux/highmem.h, or it won't see
> || the definitions of kmap_atomic/kmap_atomic_prot at all. arch/parisc itself
> || has no callers of those.
> ||
> || Outside of arch/* there is a plenty of callers. However, the following is
> || true: all instances of kmap_atomic or kmap_atomic_prot outside of arch/*
> || are either inside the linux/highmem.h or are preceded by include of
> || linux/highmem.h on any build that sees them (there is a common include
> || chain that is conditional upon CONFIG_BLOCK, but it's only needed in
> || drivers that are BLOCK-dependent). It was not fun to verify, to put
> || it mildly...
> ||
> || So for parisc we have no problem - leaving __kmap_atomic()/__kmap_atomic_prot()
> || in asm/cachefile.h and adding universal wrappers in linux/highmem.h will be
> || fine. For other architectures the things might be trickier.
And the follow up series removes kmap_* from asm/cachefile.h in parisc which
should be cleaner going forward.
> ||
> || * arc: all users in arch/arc/ are within arch/arc/mm/{cache,highmem}.c;
> || both pull linux/highmem.h. We are fine.
Still fine.
> ||
> || * arm: much, much worse. We have several files that pull linux/highmem.h:
> || arch/arm/mm/cache-feroceon-l2.c, arch/arm/mm/cache-xsc3l2.c,
> || arch/arm/mm/copypage-*.c, arch/arm/mm/dma-mapping.c, arch/arm/mm/flush.c,
> || arch/arm/mm/highmem.c, arch/arm/probes/uprobes/core.c,
> || arch/arm/include/asm/kvm_mmu.h (kmap_atomic_pfn()).
> || Those are fine, but we also have this:
> || arch/arm/include/asm/pgtable.h:200:#define __pte_map(pmd) (pte_t *)kmap_atomic(pmd_page(*(pmd)))
> || arch/arm/include/asm/pgtable.h:208:#define pte_offset_map(pmd,addr) (__pte_map(pmd) + pte_index(addr))
> || and sure as hell, asm/pgtable.h does *NOT* pull linux/highmem.h.
It does not pull asm/highmem.h either...
> ||
> || Fortunately, the users of pte_offset_map() (__pte_map() has no other users)
> || are few, both in arch/arm and outside of arch. All arm ones are pulling
> || linux/highmem (arch/arm/mm/{pgd,fault*}.c). Outside of arch we have several
> || that pull highmem.h (by way of rmap.h or pagemap.h, usually):
> || fs/userfaultfd.c, mm/gup.c, mm/hmm.c, mm/huge_memory.c,
> || mm/khugepaged.c, mm/memory-failure.c, mm/memory.c, mm/migrate.c,
> || mm/mremap.c, mm/page_vma_mapped.c, mm/swap_state.c, mm/swapfile.c,
> || mm/vmalloc.c
> || and then there are these in linux/mm.h:
> ||
> || #define pte_offset_map_lock(mm, pmd, address, ptlp) \
> || ({ \
> || spinlock_t *__ptl = pte_lockptr(mm, pmd); \
> || pte_t *__pte = pte_offset_map(pmd, address); \
> || *(ptlp) = __ptl; \
> || spin_lock(__ptl); \
> || __pte; \
> || })
> || #define pte_alloc_map(mm, pmd, address) \
> || (pte_alloc(mm, pmd) ? NULL : pte_offset_map(pmd, address))
> || #define pte_alloc_map_lock(mm, pmd, address, ptlp) \
> || (pte_alloc(mm, pmd) ? \
> || NULL : pte_offset_map_lock(mm, pmd, address, ptlp))
> ||
> || These have two users in arch/arm (arch/arm/mm/pgd.c and
> || arch/arm/lib/uaccess_with_memcpy.c, both pulling highmem.h). Outside of
> || arch there are several new files (plus a lot of what we'd already seen
> || in mm/*.c, unsurprisingly):
> || fs/proc/task_mmu.c, mm/ksm.c, mm/madvise.c, mm/memcontrol.c,
> || mm/mempolicy.c, mm/mincore.c, mm/mprotect.c, mm/pagewalk.c,
> || mm/shmem.c, mm/userfaultfd.c,
> || all pulling linux/highmem.h, as pretty much all core VM does. So we are
> || still fine.
This all seems the same now.
> ||
> || * csky: users in arch/csky/abiv2/cacheflush.c, arch/csky/mm/dma-mapping.c,
> || arch/csky/mm/highmem.c, all pulling linux/highmem.h
Yes still are.
> ||
> || * microblaze: users in arch/microblaze/mm/highmem.c (pulls linux/highmem.h) and,
> || arch/microblaze/include/asm/pgtable.h, this:
> || #define pte_offset_map(dir, addr) \
> || ((pte_t *) kmap_atomic(pmd_page(*(dir))) + pte_index(addr))
> || One pte_offset_map user in arch/microblaze:
> || arch/microblaze/kernel/signal.c:207: ptep = pte_offset_map(pmdp, address);
> || Messy, but doesn't require any changes (we have asm/pgalloc.h included
> || there, and that pull linux/highmem.h).
AFAICS asm/pgtable.h does not include asm/highmem.h here...
So looks like arch/microblaze/kernel/signal.c will need linux/highmem.h
> || Outside of arch we'd already sorted it out when looking at arm.
> ||
> || * mips: users in arch/mips/kernel/crash_dump.c, arch/mips/kernel/uprobes.c,
> || arch/mips/mm/c-r4k.c, arch/mips/mm/dma-noncoherent.c, arch/mips/mm/highmem.c,
> || and arch/mips/mm/init.c (all pulling linux/highmem.h) plus this
> || arch/mips/mm/cache.c, which relies upon asm/highmem.h. This can be switched
> || to linux/highmem.h. On !CONFIG_HIGHMEM builds the call of kmap_atomic() in
> || there is eliminated, since it's conditional upon PageHighMem(). IOW, even
> || though we get a call of (inexistent) out-of-line version, it's not going to
> || survive into object file. With linux/highmem.h use it will be an equally
> || eliminated call of inlined version.
> || XXX: arch/mips/mm/cache.c
Fixed as part of this series.
> ||
> || * nds32: users in arch/nds32/kernel/dma.c, arch/nds32/mm/cacheflush.c and
> || arch/nds32/mm/highmem.c, all pulling linux/highmem.h
Still looks ok.
> ||
> || * powerpc: users in arch/powerpc/kvm/book3s_pr.c,
> || arch/powerpc/kvm/e500_mmu_host.c, arch/powerpc/mm/dma-noncoherent.c,
> || arch/powerpc/mm/highmem.c and arch/powerpc/mm/mem.c, all pulling
> || linux/highmem.h,
still good
> a user in arch/powerpc/mm/hugetlbpage.c pulling it
> || via asm/tlb.h -> linux/pagemap.h -> linux/highmem.h
good
> and
> || macros for pte_offset_map in arch/powerpc/include/asm/*/32/pgtable.h.
> || Users of that within arch/powerpc are either 64bit-only or
> || pull linux/highmem.h (arch/powerpc/mm/pgtable_32.c and
> || arch/powerpc/xmon/xmon.c).
>
Looks ok.
> || Users outside of arch - same as for arm.
> ||
> || * sparc: users in arch/sparc/kernel/uprobes.c and arch/sparc/mm/highmem.c
> || (both pulling linux/highmem.h directly) + arch/sparc/mm/init_64.c pulling
> || it via linux/pagemap.h.
Looks ok.
> Strangely, arch/sparc/mm/io-unit.c and
> || arch/sparc/mm/iommu.c both include linux/highmem.h with odd comment
> || that seems to indicate that once upon a time pte_offset_map() used to
> || requite kmap_atomic() there... Right, it used to - until 2002.
> || These includes are pointless, then...
Looks like it...
I'll throw in a patch for that.
> ||
> || * x86: users in arch/x86/kernel/crash_dump_32.c, arch/x86/kvm/svm.c,
> || arch/x86/lib/usercopy_64.c, arch/x86/mm/highmem_32.c and arch/x86/mm/iomap_32.c,
> || all pulling linux/highmem.h, users in paging_tmpl.h (included from
> || arch/x86/kvm/mmu/mmu.c, which has pulled linux/highmem.h prior to that)
> || and definition of pte_offset_map() (in asm/pgtable_32.h)
> || Users of pte_offset_map() and friends in arch/x86 are in
> || arch/x86/kernel/vm86_32.c and arch/x86/mm/dump_pagetables.c (both
> || pulling linux/highmem.h), in arch/x86/mm/mem_encrypt_identity.c
> || (64bit-only, pte_offset_map() doesn't use kmap_atomic() there) and
> || arch/x86/kernel/tboot.c (pulls linux/highmem.h via asm/pgalloc.h
> || and linux/pagemap.h)
I've built these and they seem fine.
> ||
> || * xtensa: users in arch/xtensa/kernel/pci-dma.c, arch/xtensa/mm/highmem.c,
> || arch/xtensa/mm/cache.c and arch/xtensa/platforms/iss/simdisk.c (all pull
> || linux/highmem.h).
Actually
arch/xtensa/mm/cache.c gets linux/highmem.h from linux/pagemap.h
arch/xtensa/platforms/iss/simdisk.c may have an issue?
linux/blkdev.h -> CONFIG_BLOCK -> linux/pagemap.h -> linux/highmem.h
But simdisk.c requires BLK_DEV_SIMDISK -> CONFIG_BLOCK...
<sigh>
So xtensa still seems good AFAICS.
In summary it looks like the use of kmap_atomic() in pte_offset_map() is a
potential issue in microblaze. I've fixed that in my local tree.
Ira
next prev parent reply other threads:[~2020-05-04 20:17 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-04 1:09 [PATCH V2 00/11] Subject: Remove duplicated kmap code ira.weiny
2020-05-04 1:09 ` [PATCH V2 01/11] arch/kmap: Remove BUG_ON() ira.weiny
2020-05-04 1:09 ` [PATCH V2 02/11] arch/xtensa: Move kmap build bug out of the way ira.weiny
2020-05-04 1:09 ` [PATCH V2 03/11] arch/kmap: Remove redundant arch specific kmaps ira.weiny
2020-05-04 1:09 ` [PATCH V2 04/11] arch/kunmap: Remove duplicate kunmap implementations ira.weiny
2020-05-04 1:09 ` [PATCH V2 05/11] {x86,powerpc,microblaze}/kmap: Move preempt disable ira.weiny
2020-05-06 6:11 ` Christoph Hellwig
2020-05-06 20:25 ` Ira Weiny
2020-05-04 1:09 ` [PATCH V2 06/11] arch/kmap_atomic: Consolidate duplicate code ira.weiny
2020-05-06 6:12 ` Christoph Hellwig
2020-05-04 1:09 ` [PATCH V2 07/11] arch/kunmap_atomic: " ira.weiny
2020-05-06 6:12 ` Christoph Hellwig
2020-05-04 1:09 ` [PATCH V2 08/11] arch/kmap: Ensure kmap_prot visibility ira.weiny
2020-05-06 6:13 ` Christoph Hellwig
2020-05-06 20:33 ` Ira Weiny
2020-05-07 4:56 ` Christoph Hellwig
2020-05-04 1:09 ` [PATCH V2 09/11] arch/kmap: Don't hard code kmap_prot values ira.weiny
2020-05-04 1:09 ` [PATCH V2 10/11] arch/kmap: Define kmap_atomic_prot() for all arch's ira.weiny
2020-05-06 6:14 ` Christoph Hellwig
2020-05-04 1:09 ` [PATCH V2 11/11] drm: Remove drm specific kmap_atomic code ira.weiny
2020-05-04 11:18 ` Daniel Vetter
2020-05-04 20:24 ` Ira Weiny
2020-05-04 1:35 ` [PATCH V2 00/11] Subject: Remove duplicated kmap code Al Viro
2020-05-04 5:04 ` Ira Weiny
2020-05-04 5:33 ` Al Viro
2020-05-04 20:17 ` Ira Weiny [this message]
2020-05-04 21:02 ` Al Viro
2020-05-04 23:27 ` Ira Weiny
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=20200504201740.GA985739@iweiny-DESK2.sc.intel.com \
--to=ira.weiny@intel.com \
--cc=James.Bottomley@hansenpartnership.com \
--cc=akpm@linux-foundation.org \
--cc=benh@kernel.crashing.org \
--cc=bp@alien8.de \
--cc=chris@zankel.net \
--cc=christian.koenig@amd.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=davem@davemloft.net \
--cc=deller@gmx.de \
--cc=dri-devel@lists.freedesktop.org \
--cc=hpa@zytor.com \
--cc=jcmvbkbc@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-csky@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-parisc@vger.kernel.org \
--cc=linux-snps-arc@lists.infradead.org \
--cc=linux-xtensa@linux-xtensa.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=paulus@samba.org \
--cc=peterz@infradead.org \
--cc=ray.huang@amd.com \
--cc=sparclinux@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=tsbogend@alpha.franken.de \
--cc=viro@zeniv.linux.org.uk \
--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
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).