Re: [PATCH 14/14] mm: turn on vm_fault_t type checking
On Wed, May 16, 2018 at 08:08:29AM -0700, Darrick J. Wong wrote: > Uh, we're changing function signatures /and/ redefinining vm_fault_t? > All in the same 90K patch? > > I /was/ expecting a series of "convert X and all callers/users" > patches followed by a trivial one to switch the definition, not a giant > pile of change. FWIW I don't mind so much if you make a patch > containing a change for some super-common primitive and a hojillion > little diff hunks tree-wide, but only one logical change at a time for a > big patch, please... > > I quite prefer seeing the whole series from start to finish all packaged > up in one series, but wow this was overwhelming. :/ Another vote to split the change of the typedef, ok I get the message.. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/14] mm: turn on vm_fault_t type checking
On Wed, May 16, 2018 at 07:43:48AM +0200, Christoph Hellwig wrote:
> Switch vm_fault_t to point to an unsigned int with __bіtwise annotations.
> This both catches any old ->fault or ->page_mkwrite instance with plain
> compiler type checking, as well as finding more intricate problems with
> sparse.
>
> Signed-off-by: Christoph Hellwig
> ---
For the iomap and xfs parts,
Reviewed-by: Darrick J. Wong
That said...
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 54f1e05ecf3e..da2b77a19911 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -22,7 +22,8 @@
> #endif
> #define AT_VECTOR_SIZE (2*(AT_VECTOR_SIZE_ARCH + AT_VECTOR_SIZE_BASE + 1))
>
> -typedef int vm_fault_t;
> +typedef unsigned __bitwise vm_fault_t;
> +
>
> struct address_space;
> struct mem_cgroup;
> @@ -619,7 +620,7 @@ struct vm_special_mapping {
>* If non-NULL, then this is called to resolve page faults
>* on the special mapping. If used, .pages is not checked.
>*/
> - int (*fault)(const struct vm_special_mapping *sm,
> + vm_fault_t (*fault)(const struct vm_special_mapping *sm,
Uh, we're changing function signatures /and/ redefinining vm_fault_t?
All in the same 90K patch?
I /was/ expecting a series of "convert X and all callers/users"
patches followed by a trivial one to switch the definition, not a giant
pile of change. FWIW I don't mind so much if you make a patch
containing a change for some super-common primitive and a hojillion
little diff hunks tree-wide, but only one logical change at a time for a
big patch, please...
I quite prefer seeing the whole series from start to finish all packaged
up in one series, but wow this was overwhelming. :/
--D
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/14] mm: turn on vm_fault_t type checking
On Wed, May 16, 2018 at 04:28:13AM -0700, Matthew Wilcox wrote: > On Wed, May 16, 2018 at 07:43:48AM +0200, Christoph Hellwig wrote: > > Switch vm_fault_t to point to an unsigned int with __bіtwise annotations. > > This both catches any old ->fault or ->page_mkwrite instance with plain > > compiler type checking, as well as finding more intricate problems with > > sparse. > > Come on, Christoph; you know better than this. This patch is completely > unreviewable. Split it into one patch per maintainer tree, and in any > event, the patch to convert vm_fault_t to an unsigned int should be > separated from all the trivial conversions. The whole point is that tiny split patches for mechnical translations are totally pointless. Switching the typedef might be worth splitting if people really insist. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/14] mm: turn on vm_fault_t type checking
On Wed, May 16, 2018 at 07:43:48AM +0200, Christoph Hellwig wrote: > Switch vm_fault_t to point to an unsigned int with __bіtwise annotations. > This both catches any old ->fault or ->page_mkwrite instance with plain > compiler type checking, as well as finding more intricate problems with > sparse. Come on, Christoph; you know better than this. This patch is completely unreviewable. Split it into one patch per maintainer tree, and in any event, the patch to convert vm_fault_t to an unsigned int should be separated from all the trivial conversions. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/14] mm: turn on vm_fault_t type checking
Switch vm_fault_t to point to an unsigned int with __bіtwise annotations. This both catches any old ->fault or ->page_mkwrite instance with plain compiler type checking, as well as finding more intricate problems with sparse. Signed-off-by: Christoph Hellwig --- arch/alpha/mm/fault.c | 2 +- arch/arc/mm/fault.c | 3 +- arch/arm/mm/fault.c | 5 +- arch/arm64/mm/fault.c | 7 +- arch/hexagon/mm/vm_fault.c| 2 +- arch/ia64/mm/fault.c | 2 +- arch/m68k/mm/fault.c | 2 +- arch/microblaze/mm/fault.c| 2 +- arch/mips/mm/fault.c | 2 +- arch/nds32/mm/fault.c | 2 +- arch/nios2/mm/fault.c | 2 +- arch/openrisc/mm/fault.c | 2 +- arch/parisc/mm/fault.c| 2 +- arch/powerpc/include/asm/copro.h | 2 +- arch/powerpc/mm/copro_fault.c | 2 +- arch/powerpc/mm/fault.c | 10 +-- arch/powerpc/platforms/cell/spufs/fault.c | 2 +- arch/riscv/mm/fault.c | 3 +- arch/s390/kernel/vdso.c | 2 +- arch/s390/mm/fault.c | 2 +- arch/sh/mm/fault.c| 2 +- arch/sparc/mm/fault_32.c | 4 +- arch/sparc/mm/fault_64.c | 3 +- arch/um/kernel/trap.c | 2 +- arch/unicore32/mm/fault.c | 10 +-- arch/x86/entry/vdso/vma.c | 4 +- arch/x86/mm/fault.c | 11 +-- arch/xtensa/mm/fault.c| 2 +- drivers/dax/device.c | 21 +++--- drivers/gpu/drm/drm_vm.c | 10 +-- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_gem.h | 2 +- drivers/gpu/drm/gma500/framebuffer.c | 6 +- drivers/gpu/drm/gma500/gem.c | 2 +- drivers/gpu/drm/gma500/psb_drv.h | 2 +- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gem.c | 21 ++ drivers/gpu/drm/msm/msm_drv.h | 2 +- drivers/gpu/drm/msm/msm_gem.c | 2 +- drivers/gpu/drm/qxl/qxl_ttm.c | 4 +- drivers/gpu/drm/radeon/radeon_ttm.c | 2 +- drivers/gpu/drm/udl/udl_drv.h | 2 +- drivers/gpu/drm/udl/udl_gem.c | 2 +- drivers/gpu/drm/vc4/vc4_bo.c | 2 +- drivers/gpu/drm/vc4/vc4_drv.h | 2 +- drivers/hwtracing/intel_th/msu.c | 2 +- drivers/iommu/amd_iommu_v2.c | 2 +- drivers/iommu/intel-svm.c | 3 +- drivers/misc/cxl/fault.c | 2 +- drivers/misc/ocxl/context.c | 6 +- drivers/misc/ocxl/link.c | 2 +- drivers/misc/ocxl/sysfs.c | 2 +- drivers/scsi/cxlflash/superpipe.c | 4 +- drivers/staging/ncpfs/mmap.c | 2 +- drivers/xen/privcmd.c | 2 +- fs/9p/vfs_file.c | 2 +- fs/afs/internal.h | 2 +- fs/afs/write.c| 2 +- fs/f2fs/file.c| 10 +-- fs/fuse/file.c| 2 +- fs/gfs2/file.c| 2 +- fs/iomap.c| 2 +- fs/nfs/file.c | 4 +- fs/nilfs2/file.c | 2 +- fs/proc/vmcore.c | 2 +- fs/userfaultfd.c | 4 +- fs/xfs/xfs_file.c | 12 ++-- include/linux/huge_mm.h | 13 ++-- include/linux/hugetlb.h | 2 +- include/linux/iomap.h | 4 +- include/linux/mm.h| 67 + include/linux/mm_types.h | 5 +- include/linux/oom.h | 2 +- include/linux/swapops.h | 4 +- include/linux/userfaultfd_k.h | 5 +- ipc/shm.c | 2 +- kernel/events/core.c | 4 +- mm/gup.c | 7 +- mm/hmm.c | 2 +- mm/huge_memory.c | 29 mm/hugetlb.c | 25 +++ mm/internal.h | 2 +- mm/khugepaged.c | 3 +- mm/ksm.c | 2 +- mm/memory.c | 88 --- mm/mmap.c | 4 +- mm/shmem.c| 9 +-- samples/vfio-mdev/mbochs.c| 4 +- virt/kvm/kvm_main.c | 2 +- 91 files changed, 285 inser
