mm: inititalize struct pages when adding a section

Right now, struct pages are inititalized when memory is onlined, not when it is added (since commit d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")).

remove_memory() will call arch_remove_memory(). Here, we usually access the struct page to get the zone of the pages.

So effectively, we access stale struct pages in case we remove memory that was never onlined.

So let's simply inititalize them earlier, when the
memory is added. We only have to take care of updating the zone once we know it. We can use a dummy zone for that purpose.

So effectively, all pages will already be initialized and set to reserved after memory was added but before it was onlined (and even the memblock is added). We only inititalize pages once, to not degrade performance.

include/linux/mm.h +0 -0

@@ -1162,7 +1162,15 @@ static inline void set_page_address(struct page *page, void *address)
 {
        page->virtual = address;
 }

include/linux/mm.h +0 -0

 #endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
 

mm/memory_hotplug.c +7 -0

index 7deb49f69e27..3f28ca3c3a33 100644
@@ -250,6 +250,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
                struct vmem_altmap *altmap, bool want_memblock)
 {
        int ret;
+       int i;
 
        if (pfn_valid(phys_start_pfn))
                return -EEXIST;

@@ -258,6 +259,23 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
        if (ret < 0)
                return ret;
 
+       /*
+        * Initialize all pages in the section before fully exposing them to the
+        * system so nobody will stumble over a half inititalized state.
+        */
+       for (i = 0; i < PAGES_PER_SECTION; i++) {
+               unsigned long pfn = phys_start_pfn + i;

mm/memory_hotplug.c +0 -0

@@ -5519,9 +5515,12 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
 
 not_early:
                page = pfn_to_page(pfn);

Hi David,

Quoted 10 lines · David Hildenbrand

Right now, struct pages are inititalized when memory is onlined, not when it is added (since commit d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")).

remove_memory() will call arch_remove_memory(). Here, we usually access the struct page to get the zone of the pages.

So effectively, we access stale struct pages in case we remove memory that was never onlined.

Yeah, this is a bug, thank you for catching it.

Quoted 8 lines

So let's simply inititalize them earlier, when the
memory is added. We only have to take care of updating the zone once we know it. We can use a dummy zone for that purpose.

So effectively, all pages will already be initialized and set to reserved after memory was added but before it was onlined (and even the memblock is added). We only inititalize pages once, to not degrade performance.

Yes, but we still add one more npages loop, so there will be some performance degradation, but not severe.

There are many conflicts with linux-next, please sync before sending out next patch.

mm/page_alloc.c void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone, · line 5523

Quoted 12 lines

not_early:

                page = pfn_to_page(pfn);

mm/page_alloc.c +6 -3

-               __init_single_page(page, pfn, zone, nid);
-               if (context == MEMMAP_HOTPLUG)
-                       SetPageReserved(page);
+               if (context == MEMMAP_HOTPLUG) {
+                       /* everything but the zone was inititalized */
+                       set_page_zone(page, zone);
+                       set_page_virtual(page, zone);
+               } else
+                       init_single_page(page, pfn, zone, nid);

Please add a new function:
memmap_init_zone_hotplug() that will handle only the zone and virtual fields for onlined hotplug memory.

Please remove: "enum memmap_context context" from everywhere.

Right now, struct pages are inititalized when memory is onlined, not when it is added (since commit d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")).

remove_memory() will call arch_remove_memory(). Here, we usually access the struct page to get the zone of the pages.

So effectively, we access stale struct pages in case we remove memory that was never onlined. So let's simply inititalize them earlier, when the memory is added. We only have to take care of updating the zone once we know it. We can use a dummy zone for that purpose.

So effectively, all pages will already be initialized and set to reserved after memory was added but before it was onlined (and even the memblock is added). We only inititalize pages once, to not degrade performance.

Quoted 11 lines · David Hildenbrand

Right now, struct pages are inititalized when memory is onlined, not when it is added (since commit d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")).

remove_memory() will call arch_remove_memory(). Here, we usually access the struct page to get the zone of the pages.

So effectively, we access stale struct pages in case we remove memory that was never onlined. So let's simply inititalize them earlier, when the memory is added. We only have to take care of updating the zone once we know it. We can use a dummy zone for that purpose.

I have considered something like this when I was reworking memory hotplug to not associate struct pages with zone before onlining and I considered this to be rather fragile. I would really not like to get back to that again if possible.

Quoted 4 lines

So effectively, all pages will already be initialized and set to reserved after memory was added but before it was onlined (and even the memblock is added). We only inititalize pages once, to not degrade performance.

To be honest, I would rather see d0dc12e86b31 reverted. It is late in the release cycle and if the patch is buggy then it should be reverted rather than worked around. I found the optimization not really convincing back then and this is still the case TBH.

Quoted 23 lines · Michal Hocko

Quoted 11 lines · David Hildenbrand

Right now, struct pages are inititalized when memory is onlined, not when it is added (since commit d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")).

remove_memory() will call arch_remove_memory(). Here, we usually access the struct page to get the zone of the pages.

So effectively, we access stale struct pages in case we remove memory that was never onlined. So let's simply inititalize them earlier, when the memory is added. We only have to take care of updating the zone once we know it. We can use a dummy zone for that purpose.

I have considered something like this when I was reworking memory hotplug to not associate struct pages with zone before onlining and I considered this to be rather fragile. I would really not like to get back to that again if possible.

Quoted 4 lines

So effectively, all pages will already be initialized and set to reserved after memory was added but before it was onlined (and even the memblock is added). We only inititalize pages once, to not degrade performance.

Quoted 5 lines

To be honest, I would rather see d0dc12e86b31 reverted. It is late in the release cycle and if the patch is buggy then it should be reverted rather than worked around. I found the optimization not really convincing back then and this is still the case TBH.

If I am not wrong, that's already broken in 4.17, no? What about that?

Quoted 24 lines · David Hildenbrand

Quoted 23 lines · Michal Hocko

Quoted 11 lines · David Hildenbrand

Right now, struct pages are inititalized when memory is onlined, not when it is added (since commit d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")).

remove_memory() will call arch_remove_memory(). Here, we usually access the struct page to get the zone of the pages.

So effectively, we access stale struct pages in case we remove memory that was never onlined. So let's simply inititalize them earlier, when the memory is added. We only have to take care of updating the zone once we know it. We can use a dummy zone for that purpose.

I have considered something like this when I was reworking memory hotplug to not associate struct pages with zone before onlining and I considered this to be rather fragile. I would really not like to get back to that again if possible.

Quoted 4 lines

So effectively, all pages will already be initialized and set to reserved after memory was added but before it was onlined (and even the memblock is added). We only inititalize pages once, to not degrade performance.

Quoted 7 lines

Quoted 5 lines

To be honest, I would rather see d0dc12e86b31 reverted. It is late in the release cycle and if the patch is buggy then it should be reverted rather than worked around. I found the optimization not really convincing back then and this is still the case TBH.

If I am not wrong, that's already broken in 4.17, no? What about that?

Ohh, I thought this was merged in 4.18.

$ git describe --contains d0dc12e86b31 --match="v*"
v4.17-rc1~99^2~44

proves me wrong. This means that the fix is not so urgent as I thought. If you can figure out a reasonable fix then it should be preferable to the revert.

Fake zone sounds too hackish to me though.

Quoted 34 lines · Michal Hocko

Quoted 24 lines · David Hildenbrand

Quoted 23 lines · Michal Hocko

Quoted 11 lines · David Hildenbrand

Right now, struct pages are inititalized when memory is onlined, not when it is added (since commit d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")).

remove_memory() will call arch_remove_memory(). Here, we usually access the struct page to get the zone of the pages.

So effectively, we access stale struct pages in case we remove memory that was never onlined. So let's simply inititalize them earlier, when the memory is added. We only have to take care of updating the zone once we know it. We can use a dummy zone for that purpose.

I have considered something like this when I was reworking memory hotplug to not associate struct pages with zone before onlining and I considered this to be rather fragile. I would really not like to get back to that again if possible.

Quoted 4 lines

So effectively, all pages will already be initialized and set to reserved after memory was added but before it was onlined (and even the memblock is added). We only inititalize pages once, to not degrade performance.

Quoted 7 lines

Quoted 5 lines

To be honest, I would rather see d0dc12e86b31 reverted. It is late in the release cycle and if the patch is buggy then it should be reverted rather than worked around. I found the optimization not really convincing back then and this is still the case TBH.

If I am not wrong, that's already broken in 4.17, no? What about that?

Ohh, I thought this was merged in 4.18.

Quoted 9 lines

$ git describe --contains d0dc12e86b31 --match="v*"
v4.17-rc1~99^2~44

proves me wrong. This means that the fix is not so urgent as I thought. If you can figure out a reasonable fix then it should be preferable to the revert.

Fake zone sounds too hackish to me though.

If I am not wrong, that's the same we had before d0dc12e86b31 but now it is explicit and only one single value for all kernel configs ("ZONE_NORMAL").

Before d0dc12e86b31, struct pages were initialized to 0. So it was (depending on the config) ZONE_DMA, ZONE_DMA32 or ZONE_NORMAL.

Now the value is random and might not even be a valid zone.

Quoted 53 lines · David Hildenbrand

Quoted 34 lines · Michal Hocko

Quoted 24 lines · David Hildenbrand

Quoted 23 lines · Michal Hocko

Quoted 11 lines · David Hildenbrand

Right now, struct pages are inititalized when memory is onlined, not when it is added (since commit d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")).

remove_memory() will call arch_remove_memory(). Here, we usually access the struct page to get the zone of the pages.

So effectively, we access stale struct pages in case we remove memory that was never onlined. So let's simply inititalize them earlier, when the memory is added. We only have to take care of updating the zone once we know it. We can use a dummy zone for that purpose.

I have considered something like this when I was reworking memory hotplug to not associate struct pages with zone before onlining and I considered this to be rather fragile. I would really not like to get back to that again if possible.

Quoted 4 lines

So effectively, all pages will already be initialized and set to reserved after memory was added but before it was onlined (and even the memblock is added). We only inititalize pages once, to not degrade performance.

Quoted 7 lines

Quoted 5 lines

To be honest, I would rather see d0dc12e86b31 reverted. It is late in the release cycle and if the patch is buggy then it should be reverted rather than worked around. I found the optimization not really convincing back then and this is still the case TBH.

If I am not wrong, that's already broken in 4.17, no? What about that?

Ohh, I thought this was merged in 4.18.

Quoted 9 lines

$ git describe --contains d0dc12e86b31 --match="v*"
v4.17-rc1~99^2~44

proves me wrong. This means that the fix is not so urgent as I thought. If you can figure out a reasonable fix then it should be preferable to the revert.

Fake zone sounds too hackish to me though.

If I am not wrong, that's the same we had before d0dc12e86b31 but now it is explicit and only one single value for all kernel configs ("ZONE_NORMAL").

Before d0dc12e86b31, struct pages were initialized to 0. So it was (depending on the config) ZONE_DMA, ZONE_DMA32 or ZONE_NORMAL.

Quoted 1 line

Now the value is random and might not even be a valid zone.

Hi David,

Have you figured out why we access struct pages during hot-unplug for offlined memory? Also, a panic trace would be useful in the patch.

As I understand the bug may occur only when hotremove is enabled, and default onlining of added memory is disabled. Is this correct? I

suspect the reason we have not heard about this bug is that it is rare to add memory and not to online it.

Thank you,
Pavel

On 30.07.2018 15:30, Pavel Tatashin wrote:

Quoted 4 lines

Hi David,

Have you figured out why we access struct pages during hot-unplug for offlined memory? Also, a panic trace would be useful in the patch.

__remove_pages() needs a zone as of now (e.g. to recalculate if the zone is contiguous). This zone is taken from the first page of memory to be removed. If the struct pages are uninitialized that value is random and we might even get an invalid zone.

The zone is also used to locate pgdat.

No stack trace available so far, I'm just reading the code and try to understand how this whole memory hotplug/unplug machinery works.

Quoted 1 line · David Hildenbrand

On 30.07.2018 15:30, Pavel Tatashin wrote:

[...]

Quoted 10 lines

Quoted 4 lines

Hi David,

Have you figured out why we access struct pages during hot-unplug for offlined memory? Also, a panic trace would be useful in the patch.

__remove_pages() needs a zone as of now (e.g. to recalculate if the zone is contiguous). This zone is taken from the first page of memory to be removed. If the struct pages are uninitialized that value is random and we might even get an invalid zone.

Quoted 4 lines

The zone is also used to locate pgdat.

No stack trace available so far, I'm just reading the code and try to understand how this whole memory hotplug/unplug machinery works.

Yes this is a mess (evolution of the code called otherwise ;) [1].

Functionality has been just added on top of not very well thought through bases. This is a nice example of it. We are trying to get a zone to 1) special case zone_device 2) recalculate zone state. The first shouldn't be really needed because we should simply rely on altmap. Whether it is used for zone device or not. 2) shouldn't be really needed if the section is offline and we can check that trivially.

[1] on the other hand I can see why people were reluctant to understand the mess and rather tweak their tiny thing on top...

Quoted 13 lines · Michal Hocko

Quoted 1 line · David Hildenbrand

On 30.07.2018 15:30, Pavel Tatashin wrote:

[...]

Quoted 10 lines

Quoted 4 lines

Hi David,

Have you figured out why we access struct pages during hot-unplug for offlined memory? Also, a panic trace would be useful in the patch.

__remove_pages() needs a zone as of now (e.g. to recalculate if the zone is contiguous). This zone is taken from the first page of memory to be removed. If the struct pages are uninitialized that value is random and we might even get an invalid zone.

Quoted 6 lines

Quoted 4 lines

The zone is also used to locate pgdat.

No stack trace available so far, I'm just reading the code and try to understand how this whole memory hotplug/unplug machinery works.

Yes this is a mess (evolution of the code called otherwise ;) [1].

So I guess I should not feel bad if I am having problems understanding all the details? ;)

Quoted 7 lines

Functionality has been just added on top of not very well thought through bases. This is a nice example of it. We are trying to get a zone to 1) special case zone_device 2) recalculate zone state. The first shouldn't be really needed because we should simply rely on altmap. Whether it is used for zone device or not. 2) shouldn't be really needed if the section is offline and we can check that trivially.

About 2, I am not sure if this is the case and that easy. To me it looks more like remove_pages() fixes up things that should be done in offline_pages(). Especially, if the same memory was onlined/offlined to different zones we might be in trouble (looking at code on a very high level view).

"if the section is offline and we can check that trivially" is not was is being used here. It is "of the section was online and is now offline".

Accessing a zone when removing memory sounds very wrong. offline_pages() should cleanup everything that online_pages() did.

Removing memory with pages that are still online should not be allowed. And I think this is already enforced via check_memblock_offlined_cb().

Quoted 32 lines · David Hildenbrand

Quoted 13 lines · Michal Hocko

Quoted 1 line · David Hildenbrand

On 30.07.2018 15:30, Pavel Tatashin wrote:

[...]

Quoted 10 lines

Quoted 4 lines

Hi David,

Have you figured out why we access struct pages during hot-unplug for offlined memory? Also, a panic trace would be useful in the patch.

__remove_pages() needs a zone as of now (e.g. to recalculate if the zone is contiguous). This zone is taken from the first page of memory to be removed. If the struct pages are uninitialized that value is random and we might even get an invalid zone.

Quoted 6 lines

Quoted 4 lines

The zone is also used to locate pgdat.

No stack trace available so far, I'm just reading the code and try to understand how this whole memory hotplug/unplug machinery works.

Yes this is a mess (evolution of the code called otherwise ;) [1].

So I guess I should not feel bad if I am having problems understanding all the details? ;)

Quoted 7 lines

Functionality has been just added on top of not very well thought through bases. This is a nice example of it. We are trying to get a zone to 1) special case zone_device 2) recalculate zone state. The first shouldn't be really needed because we should simply rely on altmap. Whether it is used for zone device or not. 2) shouldn't be really needed if the section is offline and we can check that trivially.

Quoted 5 lines

About 2, I am not sure if this is the case and that easy. To me it looks more like remove_pages() fixes up things that should be done in offline_pages(). Especially, if the same memory was onlined/offlined to different zones we might be in trouble (looking at code on a very high level view).

Well, this might be possible. Hotplug remove path was on my todo list for a long time. I didn't get that far TBH. shrink_zone_span begs for some attention.

So i guess we agree that the right fix for this is to not touch struct pages when removing memory, correct?

--

Thanks,

David / dhildenb