| Message ID | 20200504010912.982044-1-ira.weiny@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Subject: Remove duplicated kmap code | expand |
On Sun, May 03, 2020 at 06:09:01PM -0700, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > The kmap infrastructure has been copied almost verbatim to every architecture. > This series consolidates obvious duplicated code by defining core functions > which call into the architectures only when needed. > > Some of the k[un]map_atomic() implementations have some similarities but the > similarities were not sufficient to warrant further changes. > > In addition we remove a duplicate implementation of kmap() in DRM. > > Testing was done by 0day to cover all the architectures I can't readily > build/test. OK... Looking through my old notes on kmap unification (this winter, never went anywhere), * arch/mips/mm/cache.c ought to use linux/highmem.h, not asm/highmem.h I suspect that your series doesn't build on some configs there. Hadn't verified that, though. * kmap_atomic_to_page() is dead, but not quite gone - csky and nds32 brought the damn thing back (nds32 - only an extern). It needs killin'... * parisc is (arguably) abusing kunmap()/kunmap_atomic() for cache flushing. Replace the bulk of its highmem.h with #define ARCH_HAS_FLUSH_ON_KUNMAP #define arch_before_kunmap flush_kernel_dcache_page_addr and have default kunmap()/kunmap_atomic() do #ifdef ARCH_HAS_FLUSH_ON_KUNMAP arch_before_kunmap(page_address(page)); #endif and #ifdef ARCH_HAS_FLUSH_ON_KUNMAP arch_before_kunmap(addr); #endif resp. Kills ARCH_HAS_KMAP along with ifdefs on it, makes parisc use somewhat less hacky. I'd suggest checking various configs on mips - that's likely to cause headache. Said that, my analysis of include chains back then is pretty much worthless by now - I really hate the amount of indirect include chains leading to that sucker on some, but not all configs ;-/ IIRC, the proof that everything using kmap*/kunmap* would pull linux/highmem.h regardless of config took several hours of digging, ran for several pages and had been hopelessly brittle. arch/mips/mm/cache.c was the only exception caught by it, but these days there might be more.
On Mon, May 04, 2020 at 02:35:09AM +0100, Al Viro wrote: > On Sun, May 03, 2020 at 06:09:01PM -0700, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > > > The kmap infrastructure has been copied almost verbatim to every architecture. > > This series consolidates obvious duplicated code by defining core functions > > which call into the architectures only when needed. > > > > Some of the k[un]map_atomic() implementations have some similarities but the > > similarities were not sufficient to warrant further changes. > > > > In addition we remove a duplicate implementation of kmap() in DRM. > > > > Testing was done by 0day to cover all the architectures I can't readily > > build/test. > > OK... Looking through my old notes on kmap unification (this winter, never > went anywhere), > > * arch/mips/mm/cache.c ought to use linux/highmem.h, not asm/highmem.h > I suspect that your series doesn't build on some configs there. Hadn't > verified that, though. Yes patch 6 makes the change because kmap_atomic() was no longer declared in asm/highmem.h. I'm pretty sure 0-day caught that ... but I seem to remember noticing some oddness in that file and I did go through it by hand. > > * kmap_atomic_to_page() is dead, but not quite gone - csky and nds32 brought > the damn thing back (nds32 - only an extern). It needs killin'... Easy enough. Added as a follow on patch. > > * parisc is (arguably) abusing kunmap()/kunmap_atomic() for cache flushing. > Replace the bulk of its highmem.h with > #define ARCH_HAS_FLUSH_ON_KUNMAP > #define arch_before_kunmap flush_kernel_dcache_page_addr > and have default kunmap()/kunmap_atomic() do > #ifdef ARCH_HAS_FLUSH_ON_KUNMAP > arch_before_kunmap(page_address(page)); > #endif > and > #ifdef ARCH_HAS_FLUSH_ON_KUNMAP > arch_before_kunmap(addr); > #endif > resp. Kills ARCH_HAS_KMAP along with ifdefs on it, makes parisc use somewhat > less hacky. Agreed. Done in a follow on patch. > > I'd suggest checking various configs on mips - that's likely to cause headache. > Said that, my analysis of include chains back then is pretty much worthless > by now - I really hate the amount of indirect include chains leading to that > sucker on some, but not all configs ;-/ IIRC, the proof that everything > using kmap*/kunmap* would pull linux/highmem.h regardless of config took several > hours of digging, ran for several pages and had been hopelessly brittle. > arch/mips/mm/cache.c was the only exception caught by it, but these days > there might be more. 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. Ira
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. || || 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. || || * arc: all users in arch/arc/ are within arch/arc/mm/{cache,highmem}.c; || both pull linux/highmem.h. We are 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. || || 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. || || * csky: users in arch/csky/abiv2/cacheflush.c, arch/csky/mm/dma-mapping.c, || arch/csky/mm/highmem.c, all pulling linux/highmem.h || || * 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). || 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 || || * nds32: users in arch/nds32/kernel/dma.c, arch/nds32/mm/cacheflush.c and || arch/nds32/mm/highmem.c, all pulling linux/highmem.h || || * 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, a user in arch/powerpc/mm/hugetlbpage.c pulling it || via asm/tlb.h -> linux/pagemap.h -> linux/highmem.h 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). 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. 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... || || * 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) || || * 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).
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
On Mon, May 04, 2020 at 01:17:41PM -0700, Ira Weiny wrote: > > || * 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... No, but the users of those macros need to be considered. > > || #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 See above - line 39 in there is #include <asm/pgalloc.h> and line 14 in arch/microblaze/include/asm/pgalloc.h is #include <linux/highmem.h> It's conditional upon CONFIG_MMU in there, but so's the use of pte_offset_map() in arch/microblaze/kernel/signal.c So it shouldn't be a problem. > > || * 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> Yep - see above re major chain of indirect includes conditional upon CONFIG_BLOCK and its uses in places that only build with such configs. There's a plenty of similar considerations outside of arch/*, unfortunately...
On Mon, May 04, 2020 at 10:02:25PM +0100, Al Viro wrote: > On Mon, May 04, 2020 at 01:17:41PM -0700, Ira Weiny wrote: > > > > || * 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... > > No, but the users of those macros need to be considered. Agreed, I was trying to point out that highmem.h was being pulled from somewhere else prior to my series, sorry. > > > > || #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 > > See above - line 39 in there is > #include <asm/pgalloc.h> > and line 14 in arch/microblaze/include/asm/pgalloc.h is > #include <linux/highmem.h> > It's conditional upon CONFIG_MMU in there, but so's the use of > pte_offset_map() in arch/microblaze/kernel/signal.c > > So it shouldn't be a problem. Ah ok, I did not see that one. Ok I'll drop that change and this series should be good. I was setting up to submit another version with 3 more patches you have suggested: kmap: Remove kmap_atomic_to_page() parisc/kmap: Remove duplicate kmap code sparc: Remove unnecessary includes Would you like to see those folded in? I submitted 2 of the above as a separate series already. > > > > || * 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> > > Yep - see above re major chain of indirect includes conditional upon CONFIG_BLOCK > and its uses in places that only build with such configs. There's a plenty of > similar considerations outside of arch/*, unfortunately... Indeed. FWIW the last 2 versions of this series have had no build failures with 0-day. This series in particular just finished 164 configs without issue. Would you like me to submit a new series? With your additional patches? Ira
From: Ira Weiny <ira.weiny@intel.com> The kmap infrastructure has been copied almost verbatim to every architecture. This series consolidates obvious duplicated code by defining core functions which call into the architectures only when needed. Some of the k[un]map_atomic() implementations have some similarities but the similarities were not sufficient to warrant further changes. In addition we remove a duplicate implementation of kmap() in DRM. Testing was done by 0day to cover all the architectures I can't readily build/test. --- Changes from V1: Fix bisect-ability Update commit message and fix line lengths Remove unneded kunmap_atomic_high() declarations Remove unneded kmap_atomic_high() declarations collect reviews rebase to 5.7-rc4 Changes from V0: Define kmap_flush_tlb() and make kmap() truely arch independent. Redefine the k[un]map_atomic_* code to call into the architectures for high mem pages Ensure all architectures define kmap_prot, use it appropriately, and define kmap_atomic_prot() Remove drm implementation of kmap_atomic() Ira Weiny (11): arch/kmap: Remove BUG_ON() arch/xtensa: Move kmap build bug out of the way arch/kmap: Remove redundant arch specific kmaps arch/kunmap: Remove duplicate kunmap implementations {x86,powerpc,microblaze}/kmap: Move preempt disable arch/kmap_atomic: Consolidate duplicate code arch/kunmap_atomic: Consolidate duplicate code arch/kmap: Ensure kmap_prot visibility arch/kmap: Don't hard code kmap_prot values arch/kmap: Define kmap_atomic_prot() for all arch's drm: Remove drm specific kmap_atomic code arch/arc/include/asm/highmem.h | 15 ------- arch/arc/mm/highmem.c | 28 +++--------- arch/arm/include/asm/highmem.h | 7 --- arch/arm/mm/highmem.c | 35 +++------------ arch/csky/include/asm/highmem.h | 9 +--- arch/csky/mm/highmem.c | 43 +++++-------------- arch/microblaze/include/asm/highmem.h | 28 +----------- arch/microblaze/mm/highmem.c | 16 ++----- arch/microblaze/mm/init.c | 3 -- arch/mips/include/asm/highmem.h | 9 +--- arch/mips/mm/cache.c | 6 +-- arch/mips/mm/highmem.c | 49 ++++----------------- arch/nds32/include/asm/highmem.h | 7 --- arch/nds32/mm/highmem.c | 39 +++-------------- arch/parisc/include/asm/cacheflush.h | 4 +- arch/powerpc/include/asm/highmem.h | 29 +------------ arch/powerpc/mm/highmem.c | 21 ++------- arch/powerpc/mm/mem.c | 3 -- arch/sparc/include/asm/highmem.h | 22 ---------- arch/sparc/mm/highmem.c | 18 +++----- arch/x86/include/asm/highmem.h | 9 ---- arch/x86/mm/highmem_32.c | 50 ++------------------- arch/xtensa/include/asm/highmem.h | 27 ------------ arch/xtensa/mm/highmem.c | 22 ++++------ drivers/gpu/drm/ttm/ttm_bo_util.c | 56 ++---------------------- drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 16 +++---- include/drm/ttm/ttm_bo_api.h | 4 -- include/linux/highmem.h | 62 +++++++++++++++++++++++++-- 28 files changed, 140 insertions(+), 497 deletions(-)