[00/10] Fix confusion around MAX_ORDER

  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.

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, 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