| Message ID | 20230315113133.11326-1-kirill.shutemov@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix confusion around MAX_ORDER | expand |
On Wed, Mar 15, 2023 at 02:31:23PM +0300, Kirill A. Shutemov wrote: > MAX_ORDER currently defined as number of orders page allocator supports: > user can ask buddy allocator for page order between 0 and MAX_ORDER-1. > > This definition is counter-intuitive and lead to number of bugs all over > the kernel. > > Fix the bugs and then change the definition of MAX_ORDER to be > inclusive: the range of orders user can ask from buddy allocator is > 0..MAX_ORDER now. > Acked-by: Mel Gorman <mgorman@suse.de> Overall looks sane other than the fixups that need to be added as flagged by LKP. There is a mild risk for stable backports that reference MAX_ORDER but that's the responsibilty of who is doing the backport. There is a mild risk of muscle memory adding off-by-one errors for new code using MAX_ORDER but it's low.
From: Mel Gorman > Sent: 21 March 2023 16:39 > > On Wed, Mar 15, 2023 at 02:31:23PM +0300, Kirill A. Shutemov wrote: > > MAX_ORDER currently defined as number of orders page allocator supports: > > user can ask buddy allocator for page order between 0 and MAX_ORDER-1. > > > > This definition is counter-intuitive and lead to number of bugs all over > > the kernel. > > > > Fix the bugs and then change the definition of MAX_ORDER to be > > inclusive: the range of orders user can ask from buddy allocator is > > 0..MAX_ORDER now. > > > > Acked-by: Mel Gorman <mgorman@suse.de> > > Overall looks sane other than the fixups that need to be added as > flagged by LKP. There is a mild risk for stable backports that reference > MAX_ORDER but that's the responsibilty of who is doing the backport. > There is a mild risk of muscle memory adding off-by-one errors for new > code using MAX_ORDER but it's low. How many of the places that use MAX_ORDER weren't touched? Is it actually worth changing the name at the same time. That will stop stable backport issues. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 3/15/23 12:31, Kirill A. Shutemov wrote: > MAX_ORDER currently defined as number of orders page allocator supports: > user can ask buddy allocator for page order between 0 and MAX_ORDER-1. > > This definition is counter-intuitive and lead to number of bugs all over > the kernel. > > Fix the bugs and then change the definition of MAX_ORDER to be > inclusive: the range of orders user can ask from buddy allocator is > 0..MAX_ORDER now. Looks like this crossed with three changes that introduced new uses of MAX_ORDER: drivers/accel/qaic/qaic_data.c: max_order = min(MAX_ORDER - 1, get_order(size)); drivers/md/dm-crypt.c: unsigned int order = MAX_ORDER - 1; drivers/md/dm-flakey.c: order = MAX_ORDER - 1; The bugs are all benign, MAX_ORDER - 1 can simply be changed to MAX_ORDER to be consistent with the new world order. CCing relevant maintainers... Paolo > Kirill A. Shutemov (10): > sparc/mm: Fix MAX_ORDER usage in tsb_grow() > um: Fix MAX_ORDER usage in linux_main() > floppy: Fix MAX_ORDER usage > drm/i915: Fix MAX_ORDER usage in i915_gem_object_get_pages_internal() > genwqe: Fix MAX_ORDER usage > perf/core: Fix MAX_ORDER usage in rb_alloc_aux_page() > mm/page_reporting: Fix MAX_ORDER usage in page_reporting_register() > mm/slub: Fix MAX_ORDER usage in calculate_order() > iommu: Fix MAX_ORDER usage in __iommu_dma_alloc_pages() > mm, treewide: Redefine MAX_ORDER sanely > > .../admin-guide/kdump/vmcoreinfo.rst | 2 +- > .../admin-guide/kernel-parameters.txt | 2 +- > arch/arc/Kconfig | 4 +- > arch/arm/Kconfig | 9 ++--- > arch/arm/configs/imx_v6_v7_defconfig | 2 +- > arch/arm/configs/milbeaut_m10v_defconfig | 2 +- > arch/arm/configs/oxnas_v6_defconfig | 2 +- > arch/arm/configs/pxa_defconfig | 2 +- > arch/arm/configs/sama7_defconfig | 2 +- > arch/arm/configs/sp7021_defconfig | 2 +- > arch/arm64/Kconfig | 27 ++++++------- > arch/arm64/include/asm/sparsemem.h | 2 +- > arch/arm64/kvm/hyp/include/nvhe/gfp.h | 2 +- > arch/arm64/kvm/hyp/nvhe/page_alloc.c | 10 ++--- > arch/csky/Kconfig | 2 +- > arch/ia64/Kconfig | 8 ++-- > arch/ia64/include/asm/sparsemem.h | 4 +- > arch/ia64/mm/hugetlbpage.c | 2 +- > arch/loongarch/Kconfig | 15 +++----- > arch/m68k/Kconfig.cpu | 5 +-- > arch/mips/Kconfig | 19 ++++------ > arch/nios2/Kconfig | 7 +--- > arch/powerpc/Kconfig | 27 ++++++------- > arch/powerpc/configs/85xx/ge_imp3a_defconfig | 2 +- > arch/powerpc/configs/fsl-emb-nonhw.config | 2 +- > arch/powerpc/mm/book3s64/iommu_api.c | 2 +- > arch/powerpc/mm/hugetlbpage.c | 2 +- > arch/powerpc/platforms/powernv/pci-ioda.c | 2 +- > arch/sh/configs/ecovec24_defconfig | 2 +- > arch/sh/mm/Kconfig | 17 ++++----- > arch/sparc/Kconfig | 5 +-- > arch/sparc/kernel/pci_sun4v.c | 2 +- > arch/sparc/kernel/traps_64.c | 2 +- > arch/sparc/mm/tsb.c | 4 +- > arch/xtensa/Kconfig | 5 +-- > drivers/base/regmap/regmap-debugfs.c | 8 ++-- > drivers/block/floppy.c | 2 +- > drivers/crypto/ccp/sev-dev.c | 2 +- > drivers/crypto/hisilicon/sgl.c | 6 +-- > .../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- > drivers/gpu/drm/ttm/ttm_pool.c | 22 +++++------ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +- > drivers/iommu/dma-iommu.c | 4 +- > drivers/irqchip/irq-gic-v3-its.c | 4 +- > drivers/md/dm-bufio.c | 2 +- > drivers/misc/genwqe/card_utils.c | 2 +- > .../net/ethernet/hisilicon/hns3/hns3_enet.c | 2 +- > drivers/net/ethernet/ibm/ibmvnic.h | 2 +- > drivers/video/fbdev/hyperv_fb.c | 4 +- > drivers/video/fbdev/vermilion/vermilion.c | 2 +- > drivers/virtio/virtio_balloon.c | 2 +- > drivers/virtio/virtio_mem.c | 12 +++--- > fs/ramfs/file-nommu.c | 2 +- > include/drm/ttm/ttm_pool.h | 2 +- > include/linux/hugetlb.h | 2 +- > include/linux/mmzone.h | 10 ++--- > include/linux/pageblock-flags.h | 4 +- > include/linux/slab.h | 6 +-- > kernel/crash_core.c | 2 +- > kernel/dma/pool.c | 6 +-- > mm/Kconfig | 6 +-- > mm/compaction.c | 8 ++-- > mm/debug_vm_pgtable.c | 4 +- > mm/huge_memory.c | 2 +- > mm/hugetlb.c | 4 +- > mm/kmsan/init.c | 6 +-- > mm/memblock.c | 2 +- > mm/memory_hotplug.c | 4 +- > mm/page_alloc.c | 38 +++++++++---------- > mm/page_isolation.c | 12 +++--- > mm/page_owner.c | 6 +-- > mm/page_reporting.c | 4 +- > mm/shuffle.h | 2 +- > mm/slab.c | 2 +- > mm/slub.c | 4 +- > mm/vmscan.c | 2 +- > mm/vmstat.c | 14 +++---- > net/smc/smc_ib.c | 2 +- > security/integrity/ima/ima_crypto.c | 2 +- > tools/testing/memblock/linux/mmzone.h | 6 +-- > 80 files changed, 210 insertions(+), 240 deletions(-) >
On Wed, 27 Sep 2023, Paolo Bonzini wrote: > On 3/15/23 12:31, Kirill A. Shutemov wrote: > > MAX_ORDER currently defined as number of orders page allocator supports: > > user can ask buddy allocator for page order between 0 and MAX_ORDER-1. > > > > This definition is counter-intuitive and lead to number of bugs all over > > the kernel. > > > > Fix the bugs and then change the definition of MAX_ORDER to be > > inclusive: the range of orders user can ask from buddy allocator is > > 0..MAX_ORDER now. I think that exclusive MAX_ORDER is more intuitive in the C language - i.e. if you write "for (i = 0; i < MAX_ORDER; i++)", you are supposed to loop over all allowed values. If you declare an array "void *array[MAX_ORDER];" you are supposed to hold a value for each allowed order. Pascal has for loops and array dimensions with inclusive ranges - and it is more prone to off-by-one errors. Mikulas
On 9/28/23 09:50, Mikulas Patocka wrote: >>> Fix the bugs and then change the definition of MAX_ORDER to be >>> inclusive: the range of orders user can ask from buddy allocator is >>> 0..MAX_ORDER now. > I think that exclusive MAX_ORDER is more intuitive in the C language - > i.e. if you write "for (i = 0; i < MAX_ORDER; i++)", you are supposed to > loop over all allowed values. If you declare an array "void > *array[MAX_ORDER];" you are supposed to hold a value for each allowed > order. > > Pascal has for loops and array dimensions with inclusive ranges - and it > is more prone to off-by-one errors. I agree it's somewhat confusing either way but the ship has sailed, the patch has been included in Linux for several months. Paolo
On Thu 2023-09-28 18:57:18, Paolo Bonzini wrote: > On 9/28/23 09:50, Mikulas Patocka wrote: > > > > Fix the bugs and then change the definition of MAX_ORDER to be > > > > inclusive: the range of orders user can ask from buddy allocator is > > > > 0..MAX_ORDER now. > > I think that exclusive MAX_ORDER is more intuitive in the C language - > > i.e. if you write "for (i = 0; i < MAX_ORDER; i++)", you are supposed to > > loop over all allowed values. If you declare an array "void > > *array[MAX_ORDER];" you are supposed to hold a value for each allowed > > order. > > > > Pascal has for loops and array dimensions with inclusive ranges - and it > > is more prone to off-by-one errors. > > I agree it's somewhat confusing either way but the ship has sailed, the > patch has been included in Linux for several months. Just make sure people don't backport it to stable. Fixes: (the commit that causes the semantic change) should do the trick. BR, Pavel