Skip to contentSkip to conversation

mm/pageblock: throw compiling time error if pageblock_bits can not hold MIGRATE_TYPES

PATCH Superseded Pingfan Liu 3 participants 6 messages Dec 7, 2018 – Dec 10, 2018

Subsystems (2)
Memory management Page allocator

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Pingfan Liu

author
#0 lore raw

PATCH v2 1/1 mm/pageblock: throw compiling time error if pageblock_bits can not hold MIGRATE_TYPES

Currently, NR_PAGEBLOCK_BITS and MIGRATE_TYPES are not associated by code. If someone adds extra migrate type, then he may forget to enlarge the NR_PAGEBLOCK_BITS. Hence it requires some way to fix. NR_PAGEBLOCK_BITS depends on MIGRATE_TYPES, while these macro spread on two different .h file with reverse dependency, it is a little hard to refer to MIGRATE_TYPES in pageblock-flag.h. This patch tries to remind such relation in compiling-time.

Signed-off-by Pingfan Liu <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Pavel Tatashin <[email protected]>
Cc Vlastimil Babka <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Mike Rapoport <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Alexander Duyck <[email protected]>
---
 include/linux/pageblock-flags.h | 5 +++--
 mm/page_alloc.c                 | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)
include/linux/pageblock-flags.h +3 -2
index 9132c5c..fe0aec4 100644
@@ -25,11 +25,12 @@
 
 #include <linux/types.h>
 
+#define PB_migratetype_bits 3
 /* Bit indices that affect a whole block of pages */
 enum pageblock_bits {
 	PB_migrate,
-	PB_migrate_end = PB_migrate + 3 - 1,
-			/* 3 bits required for migrate types */
+	PB_migrate_end = PB_migrate + PB_migratetype_bits - 1,
+			/* n bits required for migrate types */
 	PB_migrate_skip,/* If set the block is skipped by compaction */
 
 	/*
mm/page_alloc.c +2 -1
index 2ec9cc4..1a22d8d 100644
@@ -425,7 +425,8 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
 	unsigned long bitidx, word_bitidx;
 	unsigned long old_word, word;
 
-	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);
+	BUILD_BUG_ON(order_base_2(MIGRATE_TYPES)
+		!= (PB_migratetype_bits - 1));
 
 	bitmap = get_pageblock_bitmap(page, pfn);
 	bitidx = pfn_to_bitidx(page, pfn);
Signature · 1 line
2.7.4
#1 lore raw
Quoted 46 lines · Pingfan Liu

Currently, NR_PAGEBLOCK_BITS and MIGRATE_TYPES are not associated by code. If someone adds extra migrate type, then he may forget to enlarge the NR_PAGEBLOCK_BITS. Hence it requires some way to fix. NR_PAGEBLOCK_BITS depends on MIGRATE_TYPES, while these macro spread on two different .h file with reverse dependency, it is a little hard to refer to MIGRATE_TYPES in pageblock-flag.h. This patch tries to remind such relation in compiling-time.

Signed-off-by Pingfan Liu <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Pavel Tatashin <[email protected]>
Cc Vlastimil Babka <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Mike Rapoport <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Alexander Duyck <[email protected]>
---
 include/linux/pageblock-flags.h | 5 +++--
 mm/page_alloc.c                 | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)
include/linux/pageblock-flags.h +3 -2
index 9132c5c..fe0aec4 100644
@@ -25,11 +25,12 @@
 
 #include <linux/types.h>
 
+#define PB_migratetype_bits 3
 /* Bit indices that affect a whole block of pages */
 enum pageblock_bits {
 	PB_migrate,
-	PB_migrate_end = PB_migrate + 3 - 1,
-			/* 3 bits required for migrate types */
+	PB_migrate_end = PB_migrate + PB_migratetype_bits - 1,
+			/* n bits required for migrate types */
 	PB_migrate_skip,/* If set the block is skipped by compaction */
 
 	/*
mm/page_alloc.c +0 -0
index 2ec9cc4..1a22d8d 100644

mm/page_alloc.c void set_pfnblock_flags_mask(struct page *page, unsigned long flags, · line 428

Quoted 4 lines
mm/page_alloc.c +0 -1
@@ -425,7 +425,8 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
 	unsigned long bitidx, word_bitidx;
 	unsigned long old_word, word;
 
-	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);

Why delete this one? It's for something a bit different and also still valid.

mm/page_alloc.c void set_pfnblock_flags_mask(struct page *page, unsigned long flags, · line 429

Quoted 2 lines
mm/page_alloc.c +2 -0
+	BUILD_BUG_ON(order_base_2(MIGRATE_TYPES)
+		!= (PB_migratetype_bits - 1));

I think this should use the '>' operator. It's fine if there are less types than what can fit into 3 bits. AFAICS for !CONFIG_DMA and !CONFIG_MEMORY_ISOLATION there are just 4 types that fit into 2 bits...

Quoted 4 lines
mm/page_alloc.c +0 -0
 	bitmap = get_pageblock_bitmap(page, pfn);
 	bitidx = pfn_to_bitidx(page, pfn);
author
#2 lore raw
Quoted 51 lines · Vlastimil Babka
Quoted 49 lines · Pingfan Liu

Currently, NR_PAGEBLOCK_BITS and MIGRATE_TYPES are not associated by code. If someone adds extra migrate type, then he may forget to enlarge the NR_PAGEBLOCK_BITS. Hence it requires some way to fix. NR_PAGEBLOCK_BITS depends on MIGRATE_TYPES, while these macro spread on two different .h file with reverse dependency, it is a little hard to refer to MIGRATE_TYPES in pageblock-flag.h. This patch tries to remind such relation in compiling-time.

Signed-off-by Pingfan Liu <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Pavel Tatashin <[email protected]>
Cc Vlastimil Babka <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Mike Rapoport <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Alexander Duyck <[email protected]>
---
 include/linux/pageblock-flags.h | 5 +++--
 mm/page_alloc.c                 | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)
include/linux/pageblock-flags.h +3 -2
index 9132c5c..fe0aec4 100644
@@ -25,11 +25,12 @@
 
 #include <linux/types.h>
 
+#define PB_migratetype_bits 3
 /* Bit indices that affect a whole block of pages */
 enum pageblock_bits {
      PB_migrate,
-     PB_migrate_end = PB_migrate + 3 - 1,
-                     /* 3 bits required for migrate types */
+     PB_migrate_end = PB_migrate + PB_migratetype_bits - 1,
+                     /* n bits required for migrate types */
      PB_migrate_skip,/* If set the block is skipped by compaction */
 
      /*
mm/page_alloc.c +0 -0
index 2ec9cc4..1a22d8d 100644
@@ -425,7 +425,8 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
      unsigned long bitidx, word_bitidx;
      unsigned long old_word, word;
Quoted 5 lines

mm/page_alloc.c void set_pfnblock_flags_mask(struct page *page, unsigned long flags, · line 428

Quoted 1 line
mm/page_alloc.c +0 -1
-     BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);

Why delete this one? It's for something a bit different and also still valid.

I think it is dependent on PB_migratetype_bits (plus 1 dedicated bit for skip), hence the later one implies it.

Quoted 4 lines

mm/page_alloc.c void set_pfnblock_flags_mask(struct page *page, unsigned long flags, · line 429

Quoted 2 lines
mm/page_alloc.c +2 -0
+     BUILD_BUG_ON(order_base_2(MIGRATE_TYPES)
+             != (PB_migratetype_bits - 1));

I think this should use the '>' operator. It's fine if there are less

If ">" is chosen, then it allows wasted bits. I had thought it is not the purpose of the design, hence disard the ">". Otherwise, what about using warning on ">"?

Quoted 3 lines

types than what can fit into 3 bits. AFAICS for !CONFIG_DMA and !CONFIG_MEMORY_ISOLATION there are just 4 types that fit into 2 bits...

Oh, yes, you are right. How about this:
#if defined(CONFIG_DMA) || defined(CONFIG_MEMORY_ISOLATION) #define PB_migratetype_bits 3
#else
#define PB_migratetype_bits 2
#endif

Thanks for your kindly review.

Regards,
Pingfan

Quoted 5 lines
Quoted 4 lines
mm/page_alloc.c +0 -0
      bitmap = get_pageblock_bitmap(page, pfn);
      bitidx = pfn_to_bitidx(page, pfn);
#3 lore raw
Quoted 52 lines · Pingfan Liu
Quoted 51 lines · Vlastimil Babka
Quoted 49 lines · Pingfan Liu

Currently, NR_PAGEBLOCK_BITS and MIGRATE_TYPES are not associated by code. If someone adds extra migrate type, then he may forget to enlarge the NR_PAGEBLOCK_BITS. Hence it requires some way to fix. NR_PAGEBLOCK_BITS depends on MIGRATE_TYPES, while these macro spread on two different .h file with reverse dependency, it is a little hard to refer to MIGRATE_TYPES in pageblock-flag.h. This patch tries to remind such relation in compiling-time.

Signed-off-by Pingfan Liu <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Pavel Tatashin <[email protected]>
Cc Vlastimil Babka <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Mike Rapoport <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Alexander Duyck <[email protected]>
---
 include/linux/pageblock-flags.h | 5 +++--
 mm/page_alloc.c                 | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)
include/linux/pageblock-flags.h +3 -2
index 9132c5c..fe0aec4 100644
@@ -25,11 +25,12 @@
 
 #include <linux/types.h>
 
+#define PB_migratetype_bits 3
 /* Bit indices that affect a whole block of pages */
 enum pageblock_bits {
      PB_migrate,
-     PB_migrate_end = PB_migrate + 3 - 1,
-                     /* 3 bits required for migrate types */
+     PB_migrate_end = PB_migrate + PB_migratetype_bits - 1,
+                     /* n bits required for migrate types */
      PB_migrate_skip,/* If set the block is skipped by compaction */
 
      /*
mm/page_alloc.c +0 -0
index 2ec9cc4..1a22d8d 100644
@@ -425,7 +425,8 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
      unsigned long bitidx, word_bitidx;
      unsigned long old_word, word;
Quoted 8 lines
Quoted 5 lines

mm/page_alloc.c void set_pfnblock_flags_mask(struct page *page, unsigned long flags, · line 428

Quoted 1 line
mm/page_alloc.c +0 -1
-     BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);

Why delete this one? It's for something a bit different and also still valid.

I think it is dependent on PB_migratetype_bits (plus 1 dedicated bit for skip), hence the later one implies it.

No, NR_PAGEBLOCK_BITS simply to be 4 (or other number that can divide the number of bits in a word with no remainder), or set_pfnblock_flags_mask() will not function correctly.

Quoted 7 lines
Quoted 4 lines

mm/page_alloc.c void set_pfnblock_flags_mask(struct page *page, unsigned long flags, · line 429

Quoted 2 lines
mm/page_alloc.c +2 -0
+     BUILD_BUG_ON(order_base_2(MIGRATE_TYPES)
+             != (PB_migratetype_bits - 1));

I think this should use the '>' operator. It's fine if there are less

If ">" is chosen, then it allows wasted bits. I had thought it is not the purpose of the design, hence disard the ">".

We do allow wasted bits, because we need NR_PAGEBLOCK_BITS to be 4 in any case, as explained above.

Quoted 2 lines

Otherwise, what about
using warning on ">"?

I think it would be needed.

Quoted 9 lines
Quoted 3 lines

types than what can fit into 3 bits. AFAICS for !CONFIG_DMA and !CONFIG_MEMORY_ISOLATION there are just 4 types that fit into 2 bits...

Oh, yes, you are right. How about this:
#if defined(CONFIG_DMA) || defined(CONFIG_MEMORY_ISOLATION) #define PB_migratetype_bits 3
#else
#define PB_migratetype_bits 2
#endif

I think it's not necessary. Really, we need to have 4 NR_PAGEBLOCK_BITS with the current code, and this leaves us with 3 bits for migratetype. The only thing to check is whether migratetypes fit into these 3 bits, IMHO.

Quoted 9 lines

Thanks for your kindly review.

Regards,
Pingfan

Quoted 5 lines
Quoted 4 lines
mm/page_alloc.c +0 -0
      bitmap = get_pageblock_bitmap(page, pfn);
      bitidx = pfn_to_bitidx(page, pfn);
author
#4 lore raw
Quoted 60 lines · Vlastimil Babka
Quoted 58 lines · Pingfan Liu
Quoted 56 lines · Vlastimil Babka
Quoted 46 lines · Pingfan Liu

Currently, NR_PAGEBLOCK_BITS and MIGRATE_TYPES are not associated by code. If someone adds extra migrate type, then he may forget to enlarge the NR_PAGEBLOCK_BITS. Hence it requires some way to fix. NR_PAGEBLOCK_BITS depends on MIGRATE_TYPES, while these macro spread on two different .h file with reverse dependency, it is a little hard to refer to MIGRATE_TYPES in pageblock-flag.h. This patch tries to remind such relation in compiling-time.

Signed-off-by Pingfan Liu <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Pavel Tatashin <[email protected]>
Cc Vlastimil Babka <[email protected]>
Cc Oscar Salvador <[email protected]>
Cc Mike Rapoport <[email protected]>
Cc Joonsoo Kim <[email protected]>
Cc Alexander Duyck <[email protected]>
---
 include/linux/pageblock-flags.h | 5 +++--
 mm/page_alloc.c                 | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)
include/linux/pageblock-flags.h +3 -2
index 9132c5c..fe0aec4 100644
@@ -25,11 +25,12 @@
 
 #include <linux/types.h>
 
+#define PB_migratetype_bits 3
 /* Bit indices that affect a whole block of pages */
 enum pageblock_bits {
      PB_migrate,
-     PB_migrate_end = PB_migrate + 3 - 1,
-                     /* 3 bits required for migrate types */
+     PB_migrate_end = PB_migrate + PB_migratetype_bits - 1,
+                     /* n bits required for migrate types */
      PB_migrate_skip,/* If set the block is skipped by compaction */
 
      /*
mm/page_alloc.c +0 -0
index 2ec9cc4..1a22d8d 100644

mm/page_alloc.c void set_pfnblock_flags_mask(struct page *page, unsigned long flags, · line 428

Quoted 4 lines
mm/page_alloc.c +0 -1
@@ -425,7 +425,8 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
      unsigned long bitidx, word_bitidx;
      unsigned long old_word, word;
 
-     BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);

Why delete this one? It's for something a bit different and also still valid.

Quoted 7 lines
Quoted 2 lines

I think it is dependent on PB_migratetype_bits (plus 1 dedicated bit for skip), hence the later one implies it.

No, NR_PAGEBLOCK_BITS simply to be 4 (or other number that can divide the number of bits in a word with no remainder), or set_pfnblock_flags_mask() will not function correctly.

Yes, you are right. It will break the atomic.

Quoted 11 lines
Quoted 7 lines
Quoted 4 lines

mm/page_alloc.c void set_pfnblock_flags_mask(struct page *page, unsigned long flags, · line 429

Quoted 2 lines
mm/page_alloc.c +2 -0
+     BUILD_BUG_ON(order_base_2(MIGRATE_TYPES)
+             != (PB_migratetype_bits - 1));

I think this should use the '>' operator. It's fine if there are less

If ">" is chosen, then it allows wasted bits. I had thought it is not the purpose of the design, hence disard the ">".

We do allow wasted bits, because we need NR_PAGEBLOCK_BITS to be 4 in any case, as explained above.

Right!

Quoted 5 lines
Quoted 2 lines

Otherwise, what about
using warning on ">"?

I think it would be needed.

OK.

Quoted 10 lines
Quoted 9 lines
Quoted 3 lines

types than what can fit into 3 bits. AFAICS for !CONFIG_DMA and !CONFIG_MEMORY_ISOLATION there are just 4 types that fit into 2 bits...

Oh, yes, you are right. How about this:
#if defined(CONFIG_DMA) || defined(CONFIG_MEMORY_ISOLATION) #define PB_migratetype_bits 3
#else
#define PB_migratetype_bits 2
#endif

Quoted 4 lines

I think it's not necessary. Really, we need to have 4 NR_PAGEBLOCK_BITS with the current code, and this leaves us with 3 bits for migratetype. The only thing to check is whether migratetypes fit into these 3 bits, IMHO.

OK. I will update v3.

Thanks and regards,
Pingfan

build bot
#5 lore raw
In reply to Pingfan Liu
Thank you for the patch! Yet something to improve:

Hi Pingfan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc5 next-20181207]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Pingfan-Liu/mm-pageblock-throw-compiling-time-error-if-pageblock_bits-can-not-hold-MIGRATE_TYPES/20181207-161514
config: x86_64-randconfig-x017-201848 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All errors (new ones prefixed by >>):

In file included from include/asm-generic/bug.h:5:0,

                    from arch/x86/include/asm/bug.h:47,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/mm.h:9,
                    from mm/page_alloc.c:18:
   mm/page_alloc.c: In function 'set_pfnblock_flags_mask':
>> include/linux/compiler.h:372:38: error: call to '__compiletime_assert_429' declared with attribute error: BUILD_BUG_ON failed: order_base_2(MIGRATE_TYPES) != (PB_migratetype_bits - 1)
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                         ^
   include/linux/compiler.h:353:4: note: in definition of macro '__compiletime_assert'
       prefix ## suffix();    \
       ^~~~~~
   include/linux/compiler.h:372:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:69:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
     ^~~~~~~~~~~~~~~~
   mm/page_alloc.c:428:2: note: in expansion of macro 'BUILD_BUG_ON'
     BUILD_BUG_ON(order_base_2(MIGRATE_TYPES)
     ^~~~~~~~~~~~

vim +/__compiletime_assert_429 +372 include/linux/compiler.h

9a8ab1c3 Daniel Santos 2013-02-21  358  
9a8ab1c3 Daniel Santos 2013-02-21  359  #define _compiletime_assert(condition, msg, prefix, suffix) \
9a8ab1c3 Daniel Santos 2013-02-21  360  	__compiletime_assert(condition, msg, prefix, suffix)
9a8ab1c3 Daniel Santos 2013-02-21  361  
9a8ab1c3 Daniel Santos 2013-02-21  362  /**
9a8ab1c3 Daniel Santos 2013-02-21  363   * compiletime_assert - break build and emit msg if condition is false
9a8ab1c3 Daniel Santos 2013-02-21  364   * @condition: a compile-time constant condition to check
9a8ab1c3 Daniel Santos 2013-02-21  365   * @msg:       a message to emit if condition is false
9a8ab1c3 Daniel Santos 2013-02-21  366   *
9a8ab1c3 Daniel Santos 2013-02-21  367   * In tradition of POSIX assert, this macro will break the build if the
9a8ab1c3 Daniel Santos 2013-02-21  368   * supplied condition is *false*, emitting the supplied error message if the
9a8ab1c3 Daniel Santos 2013-02-21  369   * compiler has support to do so.
9a8ab1c3 Daniel Santos 2013-02-21  370   */
9a8ab1c3 Daniel Santos 2013-02-21  371  #define compiletime_assert(condition, msg) \

9a8ab1c3 Daniel Santos 2013-02-21 @372 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
9a8ab1c3 Daniel Santos 2013-02-21 373

:::::: The code at line 372 was first introduced by commit
:::::: 9a8ab1c39970a4938a72d94e6fd13be88a797590 bug.h, compiler.h: introduce compiletime_assert & BUILD_BUG_ON_MSG

:::::: TO: Daniel Santos <[email protected]> :::::: CC: Linus Torvalds <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation