Issue on reserving memory with no-map flag in DT

Hi All,

I am hitting boot failures when I did try to reserve memory with no-map flag using DT. Basically kernel just hangs with no indication of whats going on. Added some debug to find out the location, it was some where while dma mapping at kmap_atomic() in __dma_clear_buffer(). reserving.

The issue is very much identical to http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/294773.html but the memory reserve in my case is at start of the memory. I tried the same fixes on this thread but it did not help.

Platform: IFC6410 with APQ8064 which is a v7 platform with 2GB of memory starting at 0x80000000 and kernel is always loaded at 0x80200000 And am using multi_v7_defconfig.

Meminfo without memory reserve:
80000000-88dfffff : System RAM
80208000-80e5d307 : Kernel code
80f64000-810be397 : Kernel data
8a000000-8d9fffff : System RAM
8ec00000-8effffff : System RAM
8f700000-8fdfffff : System RAM
90000000-af7fffff : System RAM

DT entry:

        reserved-memory {
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;
                smem at 80000000 {
                        reg = <0x80000000 0x200000>;
                        no-map;
                };
        };

If I remove the no-map flag, then I can boot the board. But I don?t want kernel to map this memory at all, as this a IPC memory.

I just wanted to understand whats going on here, Am guessing that kernel would never touch that 2MB memory.

Does arm-kernel has limitation on unmapping/memblock_remove() such memory locations? Or
Is this a known issue?

Any pointers to debug this issue?

Before the kernel hangs it reports 2 errors like:

BUG: Bad page state in process swapper  pfn:fffa8
page:ef7fb500 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x96640253(locked|error|dirty|active|arch_1|reclaim|mlocked)
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags:
flags: 0x200041(locked|active|mlocked)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.19.0-rc3-00007-g412f9ba-dirty #816
Hardware name: Qualcomm (Flattened Device Tree)
[<c0218280>] (unwind_backtrace) from [<c0212be8>] (show_stack+0x20/0x24)
[<c0212be8>] (show_stack) from [<c0af7124>] (dump_stack+0x80/0x9c)
[<c0af7124>] (dump_stack) from [<c0301570>] (bad_page+0xc8/0x128)
[<c0301570>] (bad_page) from [<c03018a8>] (free_pages_prepare+0x168/0x1e0)
[<c03018a8>] (free_pages_prepare) from [<c030369c>] (free_hot_cold_page+0x3c/0x174)
[<c030369c>] (free_hot_cold_page) from [<c0303828>] (__free_pages+0x54/0x58)
[<c0303828>] (__free_pages) from [<c030395c>] (free_highmem_page+0x38/0x88)
[<c030395c>] (free_highmem_page) from [<c0f62d5c>] (mem_init+0x240/0x430)
[<c0f62d5c>] (mem_init) from [<c0f5db3c>] (start_kernel+0x1e4/0x3c8)

[<c0f5db3c>] (start_kernel) from [<80208074>] (0x80208074) Disabling lock debugging due to kernel taint

Full kernel log with memblock debug at http://paste.ubuntu.com/9761000/

(Adding linux-mm and relevant people because this looks like an issue there)

Quoted 79 lines · Srinivas Kandagatla

Hi All,

I am hitting boot failures when I did try to reserve memory with no-map flag using DT. Basically kernel just hangs with no indication of whats going on. Added some debug to find out the location, it was some where while dma mapping at kmap_atomic() in __dma_clear_buffer(). reserving.

The issue is very much identical to
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/294773.html
but the memory reserve in my case is at start of the memory. I tried the same fixes on this thread but it did not help.

Platform: IFC6410 with APQ8064 which is a v7 platform with 2GB of memory starting at 0x80000000 and kernel is always loaded at 0x80200000 And am using multi_v7_defconfig.

Meminfo without memory reserve:
80000000-88dfffff : System RAM
80208000-80e5d307 : Kernel code
80f64000-810be397 : Kernel data
8a000000-8d9fffff : System RAM
8ec00000-8effffff : System RAM
8f700000-8fdfffff : System RAM
90000000-af7fffff : System RAM

DT entry:

        reserved-memory {
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;
                smem at 80000000 {
                        reg = <0x80000000 0x200000>;
                        no-map;
                };
        };

If I remove the no-map flag, then I can boot the board. But I don?t want kernel to map this memory at all, as this a IPC memory.

I just wanted to understand whats going on here, Am guessing that kernel would never touch that 2MB memory.

Does arm-kernel has limitation on unmapping/memblock_remove() such memory locations?
Or
Is this a known issue?

Any pointers to debug this issue?

Before the kernel hangs it reports 2 errors like:

BUG: Bad page state in process swapper  pfn:fffa8
page:ef7fb500 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x96640253(locked|error|dirty|active|arch_1|reclaim|mlocked)
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags:
flags: 0x200041(locked|active|mlocked)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted
3.19.0-rc3-00007-g412f9ba-dirty #816
Hardware name: Qualcomm (Flattened Device Tree)
[<c0218280>] (unwind_backtrace) from [<c0212be8>] (show_stack+0x20/0x24)
[<c0212be8>] (show_stack) from [<c0af7124>] (dump_stack+0x80/0x9c)
[<c0af7124>] (dump_stack) from [<c0301570>] (bad_page+0xc8/0x128)

[<c0301570>] (bad_page) from [<c03018a8>] (free_pages_prepare+0x168/0x1e0)
[<c03018a8>] (free_pages_prepare) from [<c030369c>] (free_hot_cold_page+0x3c/0x174)
[<c030369c>] (free_hot_cold_page) from [<c0303828>] (__free_pages+0x54/0x58)
[<c0303828>] (__free_pages) from [<c030395c>] (free_highmem_page+0x38/0x88)
[<c030395c>] (free_highmem_page) from [<c0f62d5c>] (mem_init+0x240/0x430) [<c0f62d5c>] (mem_init) from [<c0f5db3c>] (start_kernel+0x1e4/0x3c8) [<c0f5db3c>] (start_kernel) from [<80208074>] (0x80208074) Disabling lock debugging due to kernel taint

Quoted 2 lines

Full kernel log with memblock debug at http://paste.ubuntu.com/9761000/

I don't have an IFC handy but I was able to reproduce the same issue on another board.
I think this is an underlying issue in mm code.

Removing the first 2MB changes the start address of the zone. This means the start
address is no longer pageblock aligned (4MB on this system). With a little digging, it looks like the issue is we're running off the end of the end of the
mem_map array because the memmap array is too small. This is similar to an issue fixed by 7c45512 mm: fix pageblock bitmap allocation and the following
fixes it for me:

mm/page_alloc.c +1 -1

index 7633c50..32d9436 100644
@@ -5012,7 +5012,7 @@ static void __init_refok alloc_node_mem_map(struct
 pglist_data *pgdat)
  #ifdef CONFIG_FLAT_NODE_MEM_MAP
         /* ia64 gets its own node_mem_map, before this, without bootmem */
         if (!pgdat->node_mem_map) {
-               unsigned long size, start, end;
+               unsigned long size, start, end, offset;
                 struct page *map;
 

                 /*

mm/page_alloc.c +2 -1

@@ -5020,10 +5020,11 @@ static void __init_refok
 alloc_node_mem_map(struct pglist_data *pgdat)
                  * aligned but the node_mem_map endpoints must be in order
                  * for the buddy allocator to function correctly.
                  */
+               offset = pgdat->node_start_pfn & (pageblock_nr_pages - 1);
                 start = pgdat->node_start_pfn & ~(MAX_ORDER_NR_PAGES - 1);
                 end = pgdat_end_pfn(pgdat);
                 end = ALIGN(end, MAX_ORDER_NR_PAGES);
-               size =  (end - start) * sizeof(struct page);
+               size =  ((end - start) + offset) * sizeof(struct page);
                 map = alloc_remap(pgdat->node_id, size);
                 if (!map)

map = memblock_virt_alloc_node_nopanic(size,

If there is agreement on this approach, I can turn this into a proper patch.

Thanks,
Laura

(Adding linux-mm and relevant people because this looks like an issue there)

Quoted 60 lines · Srinivas Kandagatla

Hi All,

I am hitting boot failures when I did try to reserve memory with no-map flag using DT. Basically kernel just hangs with no indication of whats going on. Added some debug to find out the location, it was some where while dma mapping at kmap_atomic() in __dma_clear_buffer(). reserving.

The issue is very much identical to http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/294773.html but the memory reserve in my case is at start of the memory. I tried the same fixes on this thread but it did not help.

Platform: IFC6410 with APQ8064 which is a v7 platform with 2GB of memory starting at 0x80000000 and kernel is always loaded at 0x80200000 And am using multi_v7_defconfig.

Meminfo without memory reserve:
80000000-88dfffff : System RAM
80208000-80e5d307 : Kernel code
80f64000-810be397 : Kernel data
8a000000-8d9fffff : System RAM
8ec00000-8effffff : System RAM
8f700000-8fdfffff : System RAM
90000000-af7fffff : System RAM

DT entry:

        reserved-memory {
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;
                smem at 80000000 {
                        reg = <0x80000000 0x200000>;
                        no-map;
                };
        };

If I remove the no-map flag, then I can boot the board. But I don?t want kernel to map this memory at all, as this a IPC memory.

I just wanted to understand whats going on here, Am guessing that kernel would never touch that 2MB memory.

Does arm-kernel has limitation on unmapping/memblock_remove() such memory locations? Or
Is this a known issue?

Any pointers to debug this issue?

Before the kernel hangs it reports 2 errors like:

BUG: Bad page state in process swapper  pfn:fffa8
page:ef7fb500 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x96640253(locked|error|dirty|active|arch_1|reclaim|mlocked)
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags:
flags: 0x200041(locked|active|mlocked)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.19.0-rc3-00007-g412f9ba-dirty #816
Hardware name: Qualcomm (Flattened Device Tree)
[<c0218280>] (unwind_backtrace) from [<c0212be8>] (show_stack+0x20/0x24)
[<c0212be8>] (show_stack) from [<c0af7124>] (dump_stack+0x80/0x9c)
[<c0af7124>] (dump_stack) from [<c0301570>] (bad_page+0xc8/0x128)
[<c0301570>] (bad_page) from [<c03018a8>] (free_pages_prepare+0x168/0x1e0)
[<c03018a8>] (free_pages_prepare) from [<c030369c>] (free_hot_cold_page+0x3c/0x174)
[<c030369c>] (free_hot_cold_page) from [<c0303828>] (__free_pages+0x54/0x58)
[<c0303828>] (__free_pages) from [<c030395c>] (free_highmem_page+0x38/0x88)
[<c030395c>] (free_highmem_page) from [<c0f62d5c>] (mem_init+0x240/0x430)
[<c0f62d5c>] (mem_init) from [<c0f5db3c>] (start_kernel+0x1e4/0x3c8)

Quoted 6 lines

[<c0f5db3c>] (start_kernel) from [<80208074>] (0x80208074) Disabling lock debugging due to kernel taint

Full kernel log with memblock debug at http://paste.ubuntu.com/9761000/

I don't have an IFC handy but I was able to reproduce the same issue on another board. I think this is an underlying issue in mm code.

Removing the first 2MB changes the start address of the zone. This means the start address is no longer pageblock aligned (4MB on this system). With a little digging, it looks like the issue is we're running off the end of the end of the mem_map array because the memmap array is too small. This is similar to an issue fixed by 7c45512 mm: fix pageblock bitmap allocation and the following fixes it for me:

mm/page_alloc.c +3 -2

index 7633c50..32d9436 100644
@@ -5012,7 +5012,7 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
  #ifdef CONFIG_FLAT_NODE_MEM_MAP
         /* ia64 gets its own node_mem_map, before this, without bootmem */
         if (!pgdat->node_mem_map) {
-               unsigned long size, start, end;
+               unsigned long size, start, end, offset;
                 struct page *map;
 
                 /*

@@ -5020,10 +5020,11 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
                  * aligned but the node_mem_map endpoints must be in order
                  * for the buddy allocator to function correctly.
                  */
+               offset = pgdat->node_start_pfn & (pageblock_nr_pages - 1);
                 start = pgdat->node_start_pfn & ~(MAX_ORDER_NR_PAGES - 1);
                 end = pgdat_end_pfn(pgdat);
                 end = ALIGN(end, MAX_ORDER_NR_PAGES);
-               size =  (end - start) * sizeof(struct page);
+               size =  ((end - start) + offset) * sizeof(struct page);
                 map = alloc_remap(pgdat->node_id, size);
                 if (!map)
                         map = memblock_virt_alloc_node_nopanic(size,

If there is agreement on this approach, I can turn this into a proper patch.

Thanks,
Laura

Quoted 107 lines · Laura Abbott

(Adding linux-mm and relevant people because this looks like an issue there)

Quoted 60 lines · Srinivas Kandagatla

Hi All,

I am hitting boot failures when I did try to reserve memory with no-map flag using DT. Basically kernel just hangs with no indication of whats going on. Added some debug to find out the location, it was some where while dma mapping at kmap_atomic() in __dma_clear_buffer(). reserving.

The issue is very much identical to http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/294773.html but the memory reserve in my case is at start of the memory. I tried the same fixes on this thread but it did not help.

Platform: IFC6410 with APQ8064 which is a v7 platform with 2GB of memory starting at 0x80000000 and kernel is always loaded at 0x80200000 And am using multi_v7_defconfig.

Meminfo without memory reserve:
80000000-88dfffff : System RAM

    80208000-80e5d307 : Kernel code
    80f64000-810be397 : Kernel data

8a000000-8d9fffff : System RAM
8ec00000-8effffff : System RAM
8f700000-8fdfffff : System RAM
90000000-af7fffff : System RAM

DT entry:

         reserved-memory {
                 #address-cells = <1>;
                 #size-cells = <1>;
                 ranges;
                 smem at 80000000 {
                         reg = <0x80000000 0x200000>;
                         no-map;
                 };
         };

If I remove the no-map flag, then I can boot the board. But I don?t want kernel to map this memory at all, as this a IPC memory.

I just wanted to understand whats going on here, Am guessing that kernel would never touch that 2MB memory.

Does arm-kernel has limitation on unmapping/memblock_remove() such memory locations? Or
Is this a known issue?

Any pointers to debug this issue?

Before the kernel hangs it reports 2 errors like:

BUG: Bad page state in process swapper  pfn:fffa8
page:ef7fb500 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x96640253(locked|error|dirty|active|arch_1|reclaim|mlocked)
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags:
flags: 0x200041(locked|active|mlocked)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.19.0-rc3-00007-g412f9ba-dirty #816
Hardware name: Qualcomm (Flattened Device Tree)
[<c0218280>] (unwind_backtrace) from [<c0212be8>] (show_stack+0x20/0x24)
[<c0212be8>] (show_stack) from [<c0af7124>] (dump_stack+0x80/0x9c)
[<c0af7124>] (dump_stack) from [<c0301570>] (bad_page+0xc8/0x128)
[<c0301570>] (bad_page) from [<c03018a8>] (free_pages_prepare+0x168/0x1e0)
[<c03018a8>] (free_pages_prepare) from [<c030369c>] (free_hot_cold_page+0x3c/0x174)
[<c030369c>] (free_hot_cold_page) from [<c0303828>] (__free_pages+0x54/0x58)
[<c0303828>] (__free_pages) from [<c030395c>] (free_highmem_page+0x38/0x88)
[<c030395c>] (free_highmem_page) from [<c0f62d5c>] (mem_init+0x240/0x430)
[<c0f62d5c>] (mem_init) from [<c0f5db3c>] (start_kernel+0x1e4/0x3c8)

Quoted 6 lines

[<c0f5db3c>] (start_kernel) from [<80208074>] (0x80208074) Disabling lock debugging due to kernel taint

Full kernel log with memblock debug at http://paste.ubuntu.com/9761000/

I don't have an IFC handy but I was able to reproduce the same issue on another board. I think this is an underlying issue in mm code.

Removing the first 2MB changes the start address of the zone. This means the start address is no longer pageblock aligned (4MB on this system). With a little digging, it looks like the issue is we're running off the end of the end of the mem_map array because the memmap array is too small. This is similar to an issue fixed by 7c45512 mm: fix pageblock bitmap allocation and the following fixes it for me:

mm/page_alloc.c +3 -2

index 7633c50..32d9436 100644
@@ -5012,7 +5012,7 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
   #ifdef CONFIG_FLAT_NODE_MEM_MAP
          /* ia64 gets its own node_mem_map, before this, without bootmem */
          if (!pgdat->node_mem_map) {
-               unsigned long size, start, end;
+               unsigned long size, start, end, offset;
                  struct page *map;
 
                  /*

@@ -5020,10 +5020,11 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
                   * aligned but the node_mem_map endpoints must be in order
                   * for the buddy allocator to function correctly.
                   */
+               offset = pgdat->node_start_pfn & (pageblock_nr_pages - 1);
                  start = pgdat->node_start_pfn & ~(MAX_ORDER_NR_PAGES - 1);
                  end = pgdat_end_pfn(pgdat);
                  end = ALIGN(end, MAX_ORDER_NR_PAGES);
-               size =  (end - start) * sizeof(struct page);
+               size =  ((end - start) + offset) * sizeof(struct page);
                  map = alloc_remap(pgdat->node_id, size);
                  if (!map)
                          map = memblock_virt_alloc_node_nopanic(size,

Quoted 1 line

If there is agreement on this approach, I can turn this into a proper patch.

I admit I may not see clearly through all the arch-specific layers and various config option combinations that are possible here, so I might be misinterpreting the code. But I think the problem here is not insufficient allocation size, but something else.

The code above continues by this line:

		pgdat->node_mem_map = map + (pgdat->node_start_pfn - start);

So, size for the map allocation has already been calculated aligned to MAX_ORDER_NR_PAGES before your patch, and node_mem_map points to the first actually present page, which might be offset from the perfect alignment. Your patch adds another offset to the already aligned size (but you use pageblock_nr_pages which might be lower than MAX_ORDER_NR_PAGES; this seems like a mistake in itself?). So with your patch we have map of aligned size starting from the node_mem_map. This means the last offset-worth of struct pages should be beyond what's needed to access struct page of pgdat_end_pfn(). If we need that extra padding to prevent crashing, then it looks really suspicious...

And when I look at node_mem_map usage, I see include/asm/generic/memory_model.h defines __pfn_to_page as (basically)

NODE_DATA(__nid)->node_mem_map + arch_local_page_offset(__pfn, __nid);\

and further above is a generic definition of arch_local_page_offset:

#define arch_local_page_offset(pfn, nid)        \
         ((pfn) - NODE_DATA(nid)->node_start_pfn)

So it looks correct to me without your patch. The map is allocated aligned, node_mem_map points to this map at the offset corresponding to node_start_pfn, and pfn_to_page subtracts node_start_pfn to get the offset relative to node_mem_map. We shouldn't need the extra padding by the node_start_pfn offset, unless something else is misbehaving here.

In the issue fixed by 7c45512 that you refer to, the problem was basically that the allocation didn't use aligned size, but this looks different to me?

Quoted 31 lines · Vlastimil Babka

On 01/17/2015 01:24 AM, Laura Abbott wrote:

I admit I may not see clearly through all the arch-specific layers and various config option combinations that are possible here, so I might be misinterpreting the code. But I think the problem here is not insufficient allocation size, but something else.

The code above continues by this line:

		pgdat->node_mem_map = map + (pgdat->node_start_pfn - start);

So, size for the map allocation has already been calculated aligned to MAX_ORDER_NR_PAGES before your patch, and node_mem_map points to the first actually present page, which might be offset from the perfect alignment. Your patch adds another offset to the already aligned size (but you use pageblock_nr_pages which might be lower than MAX_ORDER_NR_PAGES; this seems like a mistake in itself?). So with your patch we have map of aligned size starting from the node_mem_map. This means the last offset-worth of struct pages should be beyond what's needed to access struct page of pgdat_end_pfn(). If we need that extra padding to prevent crashing, then it looks really suspicious...

And when I look at node_mem_map usage, I see include/asm/generic/memory_model.h defines __pfn_to_page as (basically)

NODE_DATA(__nid)->node_mem_map + arch_local_page_offset(__pfn, __nid);\

and further above is a generic definition of arch_local_page_offset:

#define arch_local_page_offset(pfn, nid)        \
         ((pfn) - NODE_DATA(nid)->node_start_pfn)

Quoted 10 lines

So it looks correct to me without your patch. The map is allocated aligned, node_mem_map points to this map at the offset corresponding to node_start_pfn, and pfn_to_page subtracts node_start_pfn to get the offset relative to node_mem_map. We shouldn't need the extra padding by the node_start_pfn offset, unless something else is misbehaving here.

In the issue fixed by 7c45512 that you refer to, the problem was basically that the allocation didn't use aligned size, but this looks different to me?

With this hard coded debugging:

mm/page_alloc.c +7 -0

index 7633c50..241b870 100644
@@ -5029,6 +5029,11 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
                         map = memblock_virt_alloc_node_nopanic(size,
                                                                pgdat->node_id);
                 pgdat->node_mem_map = map + (pgdat->node_start_pfn - start);
+               pr_err(">>> node_start_pfn %lx node_end_pfn %lx\n",
+                       pgdat->node_start_pfn, pgdat_end_pfn(pgdat));
+               pr_err(">>> size calculated %lx\n", size);
+               pr_err(">>> allocated region %p-%lx\n", map, ((unsigned long)map)+size);
+
         }
  #ifndef CONFIG_NEED_MULTIPLE_NODES
         /*

@@ -5043,6 +5048,8 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
         }
  #endif
  #endif /* CONFIG_FLAT_NODE_MEM_MAP */
+       pr_err(">>> pfn %lx page %p\n", 0x200, pfn_to_page(0x200));
+       pr_err(">>> pfn %lx page %p\n", 0xbffff, pfn_to_page(0xbffff));
  }
 
  void __paginginit free_area_init_node(int nid, unsigned long *zones_size,

I get this output:

[    0.000000] >>> node_start_pfn 200 node_end_pfn c0000
[    0.000000] >>> size calculated 1800000
[    0.000000] >>> allocated region edffa000-ef7fa000
[    0.000000] >>> pfn 200 page ee002000
[    0.000000] >>> pfn bffff page ef7fdfe0

The start and end pfn values are correct but that page value is outside of the allocated region for the memory map. This is a CONFIG_FLATMEM system so we aren't actually using arch_local_page_offset at all:

#define __pfn_to_page(pfn) (mem_map + ((pfn) - ARCH_PFN_OFFSET))
#define __page_to_pfn(page) ((unsigned long)((page) - mem_map) + \
ARCH_PFN_OFFSET)

If you do the math, the array size is fine if we don't offset by the start but alloc_node_mem_map offsets assuming pfn_to_page will offset as well but this doesn't happen in CONFIG_FLATMEM.

Either alloc_node_mem_map needs to drop the offset or the pfn_to_page functions need to start adding the offset. It's worth noting that this gets corrected properly if we have CONFIG_HAVE_MEMBLOCK_NODE_MAP enabled so perhaps the fix is to unoffset for flatmem as well:

mm/page_alloc.c +0 -0

index 7633c50..271c44b 100644

mm/page_alloc.c +0 -0

  #endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */

Thanks,
Laura

Srinivas Kandagatla reported bad page messages when trying to remove the bottom 2MB on an ARM based IFC6410 board

BUG: Bad page state in process swapper  pfn:fffa8
page:ef7fb500 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x96640253(locked|error|dirty|active|arch_1|reclaim|mlocked)
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags:
flags: 0x200041(locked|active|mlocked)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.19.0-rc3-00007-g412f9ba-dirty #816
Hardware name: Qualcomm (Flattened Device Tree)
[<c0218280>] (unwind_backtrace) from [<c0212be8>] (show_stack+0x20/0x24)
[<c0212be8>] (show_stack) from [<c0af7124>] (dump_stack+0x80/0x9c)
[<c0af7124>] (dump_stack) from [<c0301570>] (bad_page+0xc8/0x128)
[<c0301570>] (bad_page) from [<c03018a8>] (free_pages_prepare+0x168/0x1e0)
[<c03018a8>] (free_pages_prepare) from [<c030369c>] (free_hot_cold_page+0x3c/0x174)
[<c030369c>] (free_hot_cold_page) from [<c0303828>] (__free_pages+0x54/0x58)
[<c0303828>] (__free_pages) from [<c030395c>] (free_highmem_page+0x38/0x88)
[<c030395c>] (free_highmem_page) from [<c0f62d5c>] (mem_init+0x240/0x430)
[<c0f62d5c>] (mem_init) from [<c0f5db3c>] (start_kernel+0x1e4/0x3c8)

[<c0f5db3c>] (start_kernel) from [<80208074>] (0x80208074) Disabling lock debugging due to kernel taint

Removing the lower 2MB made the start of the lowmem zone to no longer be page block aligned. IFC6410 uses CONFIG_FLATMEM where alloc_node_mem_map allocates memory for the mem_map. alloc_node_mem_map will offset for unaligned nodes with the assumption the pfn/page translation functions will account for the offset. The functions for CONFIG_FLATMEM do not offset however, resulting in overrunning the memmap array. Just use the allocated memmap without any offset when running with CONFIG_FLATMEM to avoid the overrun.

Signed-off-by Laura Abbott <[email protected]>

Reported-by Srinivas Kandagatla <[email protected]>

---
Srinivas, can you test this version of the patch?

---
 mm/page_alloc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

mm/page_alloc.c +3 -0

index 7633c50..33cef00 100644
@@ -5014,6 +5014,7 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
 	if (!pgdat->node_mem_map) {
 		unsigned long size, start, end;
 		struct page *map;
+		unsigned long offset = 0;
 
 		/*
 		 * The zone's endpoints aren't required to be MAX_ORDER

@@ -5021,6 +5022,8 @@ static void __init_refok alloc_node_mem_map(struct pglist_data *pgdat)
 		 * for the buddy allocator to function correctly.
 		 */
 		start = pgdat->node_start_pfn & ~(MAX_ORDER_NR_PAGES - 1);
+		if (!IS_ENABLED(CONFIG_FLATMEM))
+			offset = pgdat->node_start_pfn - start;
 		end = pgdat_end_pfn(pgdat);
 		end = ALIGN(end, MAX_ORDER_NR_PAGES);
 		size =  (end - start) * sizeof(struct page);

mm/page_alloc.c +0 -0

 	}
 #ifndef CONFIG_NEED_MULTIPLE_NODES
 	/*

Srinivas Kandagatla reported bad page messages when trying to remove the bottom 2MB on an ARM based IFC6410 board

BUG: Bad page state in process swapper  pfn:fffa8
page:ef7fb500 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x96640253(locked|error|dirty|active|arch_1|reclaim|mlocked)
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags:
flags: 0x200041(locked|active|mlocked)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.19.0-rc3-00007-g412f9ba-dirty #816
Hardware name: Qualcomm (Flattened Device Tree)
[<c0218280>] (unwind_backtrace) from [<c0212be8>] (show_stack+0x20/0x24)
[<c0212be8>] (show_stack) from [<c0af7124>] (dump_stack+0x80/0x9c)
[<c0af7124>] (dump_stack) from [<c0301570>] (bad_page+0xc8/0x128)
[<c0301570>] (bad_page) from [<c03018a8>] (free_pages_prepare+0x168/0x1e0)
[<c03018a8>] (free_pages_prepare) from [<c030369c>] (free_hot_cold_page+0x3c/0x174)
[<c030369c>] (free_hot_cold_page) from [<c0303828>] (__free_pages+0x54/0x58)
[<c0303828>] (__free_pages) from [<c030395c>] (free_highmem_page+0x38/0x88)
[<c030395c>] (free_highmem_page) from [<c0f62d5c>] (mem_init+0x240/0x430)
[<c0f62d5c>] (mem_init) from [<c0f5db3c>] (start_kernel+0x1e4/0x3c8)

[<c0f5db3c>] (start_kernel) from [<80208074>] (0x80208074) Disabling lock debugging due to kernel taint

Removing the lower 2MB made the start of the lowmem zone to no longer be page block aligned. IFC6410 uses CONFIG_FLATMEM where alloc_node_mem_map allocates memory for the mem_map. alloc_node_mem_map will offset for unaligned nodes with the assumption the pfn/page translation functions will account for the offset. The functions for CONFIG_FLATMEM do not offset however, resulting in overrunning the memmap array. Just use the allocated memmap without any offset when running with CONFIG_FLATMEM to avoid the overrun.

Quoted 24 lines · On Wed, 21 Jan 2015 17:01:40 -0800 Laura Abbott <[email protected]> wrote:

Srinivas Kandagatla reported bad page messages when trying to remove the bottom 2MB on an ARM based IFC6410 board

BUG: Bad page state in process swapper  pfn:fffa8
page:ef7fb500 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x96640253(locked|error|dirty|active|arch_1|reclaim|mlocked)
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags:
flags: 0x200041(locked|active|mlocked)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.19.0-rc3-00007-g412f9ba-dirty #816
Hardware name: Qualcomm (Flattened Device Tree)
[<c0218280>] (unwind_backtrace) from [<c0212be8>] (show_stack+0x20/0x24)
[<c0212be8>] (show_stack) from [<c0af7124>] (dump_stack+0x80/0x9c)
[<c0af7124>] (dump_stack) from [<c0301570>] (bad_page+0xc8/0x128)
[<c0301570>] (bad_page) from [<c03018a8>] (free_pages_prepare+0x168/0x1e0)
[<c03018a8>] (free_pages_prepare) from [<c030369c>] (free_hot_cold_page+0x3c/0x174)
[<c030369c>] (free_hot_cold_page) from [<c0303828>] (__free_pages+0x54/0x58)
[<c0303828>] (__free_pages) from [<c030395c>] (free_highmem_page+0x38/0x88)
[<c030395c>] (free_highmem_page) from [<c0f62d5c>] (mem_init+0x240/0x430)
[<c0f62d5c>] (mem_init) from [<c0f5db3c>] (start_kernel+0x1e4/0x3c8)

[<c0f5db3c>] (start_kernel) from [<80208074>] (0x80208074) Disabling lock debugging due to kernel taint

Quoted 9 lines

Removing the lower 2MB made the start of the lowmem zone to no longer be page block aligned. IFC6410 uses CONFIG_FLATMEM where alloc_node_mem_map allocates memory for the mem_map. alloc_node_mem_map will offset for unaligned nodes with the assumption the pfn/page translation functions will account for the offset. The functions for CONFIG_FLATMEM do not offset however, resulting in overrunning the memmap array. Just use the allocated memmap without any offset when running with CONFIG_FLATMEM to avoid the overrun.

I don't think v2 addressed Vlastimil's review comment?

Quoted 36 lines · Andrew Morton

Quoted 33 lines · On Wed, 21 Jan 2015 17:01:40 -0800 Laura Abbott <[email protected]> wrote:

Srinivas Kandagatla reported bad page messages when trying to remove the bottom 2MB on an ARM based IFC6410 board

BUG: Bad page state in process swapper  pfn:fffa8
page:ef7fb500 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x96640253(locked|error|dirty|active|arch_1|reclaim|mlocked)
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags:
flags: 0x200041(locked|active|mlocked)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.19.0-rc3-00007-g412f9ba-dirty #816
Hardware name: Qualcomm (Flattened Device Tree)
[<c0218280>] (unwind_backtrace) from [<c0212be8>] (show_stack+0x20/0x24)
[<c0212be8>] (show_stack) from [<c0af7124>] (dump_stack+0x80/0x9c)
[<c0af7124>] (dump_stack) from [<c0301570>] (bad_page+0xc8/0x128)
[<c0301570>] (bad_page) from [<c03018a8>] (free_pages_prepare+0x168/0x1e0)
[<c03018a8>] (free_pages_prepare) from [<c030369c>] (free_hot_cold_page+0x3c/0x174)
[<c030369c>] (free_hot_cold_page) from [<c0303828>] (__free_pages+0x54/0x58)
[<c0303828>] (__free_pages) from [<c030395c>] (free_highmem_page+0x38/0x88)
[<c030395c>] (free_highmem_page) from [<c0f62d5c>] (mem_init+0x240/0x430)
[<c0f62d5c>] (mem_init) from [<c0f5db3c>] (start_kernel+0x1e4/0x3c8)

[<c0f5db3c>] (start_kernel) from [<80208074>] (0x80208074) Disabling lock debugging due to kernel taint

Removing the lower 2MB made the start of the lowmem zone to no longer be page block aligned. IFC6410 uses CONFIG_FLATMEM where alloc_node_mem_map allocates memory for the mem_map. alloc_node_mem_map will offset for unaligned nodes with the assumption the pfn/page translation functions will account for the offset. The functions for CONFIG_FLATMEM do not offset however, resulting in overrunning the memmap array. Just use the allocated memmap without any offset when running with CONFIG_FLATMEM to avoid the overrun.

Quoted 2 lines

I don't think v2 addressed Vlastimil's review comment?

We're still adding the offset to node_mem_map and then subtracting it from just mem_map. Did I miss another comment somewhere?

Quoted 37 lines · Laura Abbott

Quoted 36 lines · Andrew Morton

Quoted 33 lines · On Wed, 21 Jan 2015 17:01:40 -0800 Laura Abbott <[email protected]> wrote:

Srinivas Kandagatla reported bad page messages when trying to remove the bottom 2MB on an ARM based IFC6410 board

BUG: Bad page state in process swapper  pfn:fffa8
page:ef7fb500 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x96640253(locked|error|dirty|active|arch_1|reclaim|mlocked)
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags:
flags: 0x200041(locked|active|mlocked)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.19.0-rc3-00007-g412f9ba-dirty #816
Hardware name: Qualcomm (Flattened Device Tree)
[<c0218280>] (unwind_backtrace) from [<c0212be8>] (show_stack+0x20/0x24)
[<c0212be8>] (show_stack) from [<c0af7124>] (dump_stack+0x80/0x9c)
[<c0af7124>] (dump_stack) from [<c0301570>] (bad_page+0xc8/0x128)
[<c0301570>] (bad_page) from [<c03018a8>] (free_pages_prepare+0x168/0x1e0)
[<c03018a8>] (free_pages_prepare) from [<c030369c>] (free_hot_cold_page+0x3c/0x174)
[<c030369c>] (free_hot_cold_page) from [<c0303828>] (__free_pages+0x54/0x58)
[<c0303828>] (__free_pages) from [<c030395c>] (free_highmem_page+0x38/0x88)
[<c030395c>] (free_highmem_page) from [<c0f62d5c>] (mem_init+0x240/0x430)
[<c0f62d5c>] (mem_init) from [<c0f5db3c>] (start_kernel+0x1e4/0x3c8)

[<c0f5db3c>] (start_kernel) from [<80208074>] (0x80208074) Disabling lock debugging due to kernel taint

Removing the lower 2MB made the start of the lowmem zone to no longer be page block aligned. IFC6410 uses CONFIG_FLATMEM where alloc_node_mem_map allocates memory for the mem_map. alloc_node_mem_map will offset for unaligned nodes with the assumption the pfn/page translation functions will account for the offset. The functions for CONFIG_FLATMEM do not offset however, resulting in overrunning the memmap array. Just use the allocated memmap without any offset when running with CONFIG_FLATMEM to avoid the overrun.

Quoted 5 lines

Quoted 2 lines

I don't think v2 addressed Vlastimil's review comment?

We're still adding the offset to node_mem_map and then subtracting it from just mem_map. Did I miss another comment somewhere?

Yes that was addressed, thanks. But I don't feel comfortable acking it yet, as I have no idea if we are doing the right thing for CONFIG_HAVE_MEMBLOCK_NODE_MAP && CONFIG_FLATMEM case here.

Also putting the CONFIG_FLATMEM && !CONFIG_HAVE_MEMBLOCK_NODE_MAP under the "if (page_to_pfn(mem_map) != pgdat->node_start_pfn)" will probably do the right thing, but looks like a weird test for this case here.

I have no good suggestion though, so let's CC Mel who apparently wrote the ARCH_PFN_OFFSET correction?

Quoted 13 lines · Vlastimil Babka

Quoted 7 lines · Laura Abbott

Quoted 3 lines · Andrew Morton

I don't think v2 addressed Vlastimil's review comment?

We're still adding the offset to node_mem_map and then subtracting it from just mem_map. Did I miss another comment somewhere?

Yes that was addressed, thanks. But I don't feel comfortable acking it yet, as I have no idea if we are doing the right thing for CONFIG_HAVE_MEMBLOCK_NODE_MAP && CONFIG_FLATMEM case here.

Quoted 8 lines

Also putting the CONFIG_FLATMEM && !CONFIG_HAVE_MEMBLOCK_NODE_MAP under the "if (page_to_pfn(mem_map) != pgdat->node_start_pfn)" will probably do the right thing, but looks like a weird test for this case here.

I have no good suggestion though, so let's CC Mel who apparently wrote the ARCH_PFN_OFFSET correction?

I don't recall introducing ARCH_PFN_OFFSET, are you sure it was me? I'm just back today after been offline a week so didn't review the patch but IIRC, ARCH_PFN_OFFSET deals with the case where physical memory does not start at 0. Without the offset, virtual _PAGE_OFFSET would not physical page 0. I don't recall it being related to the alignment of node 0 so if there are crashes due to misalignment of node 0 and the fix is ARCH_PFN_OFFSET related then I'm surprised.

Quoted 23 lines · Mel Gorman

Quoted 13 lines · Vlastimil Babka

Quoted 7 lines · Laura Abbott

Quoted 3 lines · Andrew Morton

I don't think v2 addressed Vlastimil's review comment?

We're still adding the offset to node_mem_map and then subtracting it from just mem_map. Did I miss another comment somewhere?

Yes that was addressed, thanks. But I don't feel comfortable acking it yet, as I have no idea if we are doing the right thing for CONFIG_HAVE_MEMBLOCK_NODE_MAP && CONFIG_FLATMEM case here.

Quoted 8 lines

Also putting the CONFIG_FLATMEM && !CONFIG_HAVE_MEMBLOCK_NODE_MAP under the "if (page_to_pfn(mem_map) != pgdat->node_start_pfn)" will probably do the right thing, but looks like a weird test for this case here.

I have no good suggestion though, so let's CC Mel who apparently wrote the ARCH_PFN_OFFSET correction?

Quoted 7 lines

I don't recall introducing ARCH_PFN_OFFSET, are you sure it was me? I'm just back today after been offline a week so didn't review the patch but IIRC, ARCH_PFN_OFFSET deals with the case where physical memory does not start at 0. Without the offset, virtual _PAGE_OFFSET would not physical page 0. I don't recall it being related to the alignment of node 0 so if there are crashes due to misalignment of node 0 and the fix is ARCH_PFN_OFFSET related then I'm surprised.

You're right that ARCH_PFN_OFFSET wasn't added by you, but by commit 467bc461d2 which was a bugfix to your commit c713216dee, which did
introduce the mem_map correction code, and after which the code looked like:

mem_map = NODE_DATA(0)->node_mem_map;

#ifdef CONFIG_ARCH_POPULATES_NODE_MAP
                if (page_to_pfn(mem_map) != pgdat->node_start_pfn)
                        mem_map -= pgdat->node_start_pfn;

#endif /* CONFIG_ARCH_POPULATES_NODE_MAP */

It's from 2006 so I can't expect you remember the details, but I had some
trouble finding out what this does. I assume it makes sure that mem_map points to struct page corresponding to pfn 0, because that's what translations using mem_map expect.
But pgdat->node_mem_map points to struct page corresponding to
pgdat->node_start_pfn, which might not be 0. So it subtracts node_start_pfn to fix that. This is OK, as the node_mem_map is allocated (in this very function) with padding so that it covers a MAX_ORDER_NR_PAGES aligned area where node_mem_map may point to the middle of it.

Commit 467bc461d2 fixed this in case the first pfn is not 0, but ARCH_PFN_OFFSET.
So mem_map points to struct page corresponding to pfn=ARCH_PFN_OFFSET, which is OK. But I still have few doubts:

1) The "if (page_to_pfn(mem_map) != pgdat->node_start_pfn)" sort of silently assumes that mem_map is allocated at the beginning of the node, i.e. at pgdat->node_start_pfn. And the only reason for this if-condition to be true, is that we haven't corrected the page_to_pfn translation, which uses mem_map. Is this assumption always OK to do? Shouldn't the if-condition be instead about pgdat->node_start_pfn not being aligned?

2) The #ifdef guard is about CONFIG_ARCH_POPULATES_NODE_MAP, which is nowadays called > CONFIG_HAVE_MEMBLOCK_NODE_MAP. But shouldn't it be #ifdef FLATMEM instead? After all, we are correcting value of mem_map based on page_to_pfn code
variant used on FLATMEM. arm doesn't define
CONFIG_ARCH_POPULATES_NODE_MAP but apparently needs this correction.

3) The node_mem_map allocation code aligns the allocation to MAX_ORDER_NR_PAGES, so the offset between the start of the allocated map and where node_mem_map points to will be up to MAX_ORDER_NR_PAGES.
However, here we subtract (in current kernel) (pgdat->node_start_pfn - ARCH_PFN_OFFSET).
That looks like another silent assumption, that pgdat->node_start_pfn is always between ARCH_PFN_OFFSET and ARCH_PFN_OFFSET + MAX_ORDER_NR_PAGES. If it were larger, the mem_map correction would subtract too much and end up below what was allocated for node_mem_map, no? The bug report behind this patch said that first 2MB of memory was reserved using "no-map flag using DT". Unless this somehow translates to ARCH_PFN_OFFSET at build time, we would underflow mem_map, right? Maybe I'm just overly paranoid here and of course ARCH_PFN_OFFSET is determined properly on arm...

If anyone can confirm my doubts or point me to what I'm missing, thanks.

Quoted 64 lines · Vlastimil Babka

Quoted 23 lines · Mel Gorman

Quoted 13 lines · Vlastimil Babka

Quoted 7 lines · Laura Abbott

Quoted 3 lines · Andrew Morton

I don't think v2 addressed Vlastimil's review comment?

We're still adding the offset to node_mem_map and then subtracting it from just mem_map. Did I miss another comment somewhere?

Yes that was addressed, thanks. But I don't feel comfortable acking it yet, as I have no idea if we are doing the right thing for CONFIG_HAVE_MEMBLOCK_NODE_MAP && CONFIG_FLATMEM case here.

Quoted 8 lines

Also putting the CONFIG_FLATMEM && !CONFIG_HAVE_MEMBLOCK_NODE_MAP under the "if (page_to_pfn(mem_map) != pgdat->node_start_pfn)" will probably do the right thing, but looks like a weird test for this case here.

I have no good suggestion though, so let's CC Mel who apparently wrote the ARCH_PFN_OFFSET correction?

Quoted 7 lines

I don't recall introducing ARCH_PFN_OFFSET, are you sure it was me? I'm just back today after been offline a week so didn't review the patch but IIRC, ARCH_PFN_OFFSET deals with the case where physical memory does not start at 0. Without the offset, virtual _PAGE_OFFSET would not physical page 0. I don't recall it being related to the alignment of node 0 so if there are crashes due to misalignment of node 0 and the fix is ARCH_PFN_OFFSET related then I'm surprised.

You're right that ARCH_PFN_OFFSET wasn't added by you, but by commit 467bc461d2 which was a bugfix to your commit c713216dee, which did
introduce the mem_map correction code, and after which the code looked like:

mem_map = NODE_DATA(0)->node_mem_map;

#ifdef CONFIG_ARCH_POPULATES_NODE_MAP
                if (page_to_pfn(mem_map) != pgdat->node_start_pfn)
                        mem_map -= pgdat->node_start_pfn;

#endif /* CONFIG_ARCH_POPULATES_NODE_MAP */

It's from 2006 so I can't expect you remember the details, but I had some
trouble finding out what this does. I assume it makes sure that mem_map points to struct page corresponding to pfn 0, because that's what translations using mem_map expect.
But pgdat->node_mem_map points to struct page corresponding to
pgdat->node_start_pfn, which might not be 0. So it subtracts node_start_pfn to fix that. This is OK, as the node_mem_map is allocated (in this very function) with padding so that it covers a MAX_ORDER_NR_PAGES aligned area where node_mem_map may point to the middle of it.

Commit 467bc461d2 fixed this in case the first pfn is not 0, but ARCH_PFN_OFFSET.
So mem_map points to struct page corresponding to pfn=ARCH_PFN_OFFSET, which is OK. But I still have few doubts:

1) The "if (page_to_pfn(mem_map) != pgdat->node_start_pfn)" sort of silently assumes that mem_map is allocated at the beginning of the node, i.e. at pgdat->node_start_pfn. And the only reason for this if-condition to be true, is that we haven't corrected the page_to_pfn translation, which uses mem_map. Is this assumption always OK to do? Shouldn't the if-condition be instead about pgdat->node_start_pfn not being aligned?

Quoted 5 lines

2) The #ifdef guard is about CONFIG_ARCH_POPULATES_NODE_MAP, which is nowadays called > CONFIG_HAVE_MEMBLOCK_NODE_MAP. But shouldn't it be #ifdef FLATMEM instead? After all, we are correcting value of mem_map based on page_to_pfn code
variant used on FLATMEM. arm doesn't define
CONFIG_ARCH_POPULATES_NODE_MAP but apparently needs this correction.

Just doing #ifdef FLATMEM doesn't work because ARCH_PFN_OFFSET doesn't seem to be picked up properly for NOMMU arches properly. Probably just missing a header somewhere.

Quoted 13 lines

3) The node_mem_map allocation code aligns the allocation to MAX_ORDER_NR_PAGES, so the offset between the start of the allocated map and where node_mem_map points to will be up to MAX_ORDER_NR_PAGES.
However, here we subtract (in current kernel) (pgdat->node_start_pfn - ARCH_PFN_OFFSET).
That looks like another silent assumption, that pgdat->node_start_pfn is always between ARCH_PFN_OFFSET and ARCH_PFN_OFFSET + MAX_ORDER_NR_PAGES. If it were larger, the mem_map correction would subtract too much and end up below what was allocated for node_mem_map, no? The bug report behind this patch said that first 2MB of memory was reserved using "no-map flag using DT". Unless this somehow translates to ARCH_PFN_OFFSET at build time, we would underflow mem_map, right? Maybe I'm just overly paranoid here and of course ARCH_PFN_OFFSET is determined properly on arm...

Quoted 1 line

If anyone can confirm my doubts or point me to what I'm missing, thanks.

ARCH_PFN_OFFSET should always be the lowest PFN in the system, otherwise I think plenty of other things are broken given how many architectures make this assumption. That said, I don't think subtracting ARCH_PFN_OFFSET makes it obvious why the adjustment is being made.

Thanks,
Laura

Reviving this thread because I don't think it ever got resolved.

Quoted 72 lines · Laura Abbott

Quoted 41 lines · Vlastimil Babka

Quoted 8 lines

I don't recall introducing ARCH_PFN_OFFSET, are you sure it was me? I'm just back today after been offline a week so didn't review the patch but IIRC, ARCH_PFN_OFFSET deals with the case where physical memory does not start at 0. Without the offset, virtual _PAGE_OFFSET would not physical page 0. I don't recall it being related to the alignment of node 0 so if there are crashes due to misalignment of node 0 and the fix is ARCH_PFN_OFFSET related then I'm surprised.

You're right that ARCH_PFN_OFFSET wasn't added by you, but by commit 467bc461d2 which was a bugfix to your commit c713216dee, which did
introduce the mem_map correction code, and after which the code looked like:

mem_map = NODE_DATA(0)->node_mem_map;

#ifdef CONFIG_ARCH_POPULATES_NODE_MAP
                if (page_to_pfn(mem_map) != pgdat->node_start_pfn)
                        mem_map -= pgdat->node_start_pfn;

#endif /* CONFIG_ARCH_POPULATES_NODE_MAP */

It's from 2006 so I can't expect you remember the details, but I had some
trouble finding out what this does. I assume it makes sure that mem_map points to struct page corresponding to pfn 0, because that's what translations using mem_map expect.
But pgdat->node_mem_map points to struct page corresponding to
pgdat->node_start_pfn, which might not be 0. So it subtracts node_start_pfn to fix that. This is OK, as the node_mem_map is allocated (in this very function) with padding so that it covers a MAX_ORDER_NR_PAGES aligned area where node_mem_map may point to the middle of it.

Commit 467bc461d2 fixed this in case the first pfn is not 0, but ARCH_PFN_OFFSET.
So mem_map points to struct page corresponding to pfn=ARCH_PFN_OFFSET, which is OK. But I still have few doubts:

1) The "if (page_to_pfn(mem_map) != pgdat->node_start_pfn)" sort of silently assumes that mem_map is allocated at the beginning of the node, i.e. at pgdat->node_start_pfn. And the only reason for this if-condition to be true, is that we haven't corrected the page_to_pfn translation, which uses mem_map. Is this assumption always OK to do? Shouldn't the if-condition be instead about pgdat->node_start_pfn not being aligned?

Quoted 5 lines

2) The #ifdef guard is about CONFIG_ARCH_POPULATES_NODE_MAP, which is nowadays called > CONFIG_HAVE_MEMBLOCK_NODE_MAP. But shouldn't it be #ifdef FLATMEM instead? After all, we are correcting value of mem_map based on page_to_pfn code
variant used on FLATMEM. arm doesn't define
CONFIG_ARCH_POPULATES_NODE_MAP but apparently needs this correction.

Just doing #ifdef FLATMEM doesn't work because ARCH_PFN_OFFSET doesn't seem to be picked up properly for NOMMU arches properly. Probably just missing a header somewhere.

Quoted 13 lines

3) The node_mem_map allocation code aligns the allocation to MAX_ORDER_NR_PAGES, so the offset between the start of the allocated map and where node_mem_map points to will be up to MAX_ORDER_NR_PAGES.
However, here we subtract (in current kernel) (pgdat->node_start_pfn - ARCH_PFN_OFFSET).
That looks like another silent assumption, that pgdat->node_start_pfn is always between ARCH_PFN_OFFSET and ARCH_PFN_OFFSET + MAX_ORDER_NR_PAGES. If it were larger, the mem_map correction would subtract too much and end up below what was allocated for node_mem_map, no? The bug report behind this patch said that first 2MB of memory was reserved using "no-map flag using DT". Unless this somehow translates to ARCH_PFN_OFFSET at build time, we would underflow mem_map, right? Maybe I'm just overly paranoid here and of course ARCH_PFN_OFFSET is determined properly on arm...

Quoted 1 line

If anyone can confirm my doubts or point me to what I'm missing, thanks.

ARCH_PFN_OFFSET should always be the lowest PFN in the system, otherwise I think plenty of other things are broken given how many architectures make this assumption. That said, I don't think subtracting ARCH_PFN_OFFSET makes it obvious why the adjustment is being made.

Quoted 3 lines

Thanks,
Laura

I was incorrect before: it isn't just NOMMU but architectures that don't use asm-generic/memory_model.h which failed to compile. I could respin with

more ifdefery around the ARCH_PFN_OFFSET if that sounds reasonable.

Thanks,
Laura