mm/sparse: reset section's mem_map when fully deactivated

PATCH Applied Pingfan Liu 4 participants 7 messages Jan 20, 2020 – Jan 25, 2020

Subsystems (2)

Memory management MM core

lore

Share & tools

lore

Pingfan Liu

author

2020-01-20 02:35 UTC #0 lore raw

PATCH v2 1/1 mm/sparse: reset section's mem_map when fully deactivated

After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"), when a mem section is fully deactivated, section_mem_map still records the section's start pfn, which is not used any more and will be reassigned during re-added.

In analogy with alloc/free pattern, it is better to clear all fields of section_mem_map.

Beside this, it breaks the user space tool "makedumpfile" [1], which makes assumption that a hot-removed section has mem_map as NULL, instead of checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be better to change the assumption, and need a patch)

The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" , trigger a crash, and save vmcore by makedumpfile

[1]: makedumpfile, commit e73016540293 ("[v1.6.7] Update version")

Signed-off-by Pingfan Liu <[email protected]>

To: [email protected]
Cc: Andrew Morton <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Oscar Salvador <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: Qian Cai <[email protected]>
Cc: [email protected]
Cc: Kazuhito Hagio <[email protected]>
---
v1 -> v2:
make an explicit convertion from NULL to ulong
improve commit log

 mm/sparse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/sparse.c +1 -1

index 3822ecb..3918fc3 100644
@@ -789,7 +789,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 			ms->usage = NULL;
 		}
 		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
-		ms->section_mem_map = sparse_encode_mem_map(NULL, section_nr);
+		ms->section_mem_map = (unsigned long)NULL;
 	}
 
 	if (section_is_early && memmap)

Signature · 1 line

2.7.5

Michal Hocko

reviewer Acked-by on v2 1/1

2020-01-20 07:29 UTC #1 lore raw

Quoted 8 lines · Pingfan Liu

After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"), when a mem section is fully deactivated, section_mem_map still records the section's start pfn, which is not used any more and will be reassigned during re-added.
In analogy with alloc/free pattern, it is better to clear all fields of section_mem_map.

Quoted 7 lines

Beside this, it breaks the user space tool "makedumpfile" [1], which makes assumption that a hot-removed section has mem_map as NULL, instead of checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be better to change the assumption, and need a patch)
The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" , trigger a crash, and save vmcore by makedumpfile

While makedumpfile lives very closely to the kernel and occasional breakage is to be expected I still believe that Fixes: ba72b4c8cf60 is due.

Quoted 13 lines

[1]: makedumpfile, commit e73016540293 ("[v1.6.7] Update version")
Signed-off-by Pingfan Liu <[email protected]>
To: [email protected]
Cc: Andrew Morton <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Oscar Salvador <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: Qian Cai <[email protected]>
Cc: [email protected]
Cc: Kazuhito Hagio <[email protected]>

Acked-by Michal Hocko <[email protected]>

Quoted 23 lines

---
v1 -> v2:
make an explicit convertion from NULL to ulong
improve commit log

 mm/sparse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/sparse.c +1 -1

index 3822ecb..3918fc3 100644
@@ -789,7 +789,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 			ms->usage = NULL;
 		}
 		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
-		ms->section_mem_map = sparse_encode_mem_map(NULL, section_nr);
+		ms->section_mem_map = (unsigned long)NULL;
 	}
 
 	if (section_is_early && memmap)

Signature · 1 line

2.7.5

Signature · 2 lines

Michal Hocko
SUSE Labs

David Hildenbrand

Acked-by

2020-01-20 09:03 UTC #2 lore raw

Quoted 14 lines · Michal Hocko

Quoted 13 lines · Pingfan Liu
After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"), when a mem section is fully deactivated, section_mem_map still records the section's start pfn, which is not used any more and will be reassigned during re-added.
In analogy with alloc/free pattern, it is better to clear all fields of section_mem_map.
Beside this, it breaks the user space tool "makedumpfile" [1], which makes assumption that a hot-removed section has mem_map as NULL, instead of checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be better to change the assumption, and need a patch)

Quoted 6 lines

Quoted 2 lines
The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" , trigger a crash, and save vmcore by makedumpfile
While makedumpfile lives very closely to the kernel and occasional breakage is to be expected I still believe that Fixes: ba72b4c8cf60 is due.

+1 as I expressed earlier.

Quoted 15 lines

Quoted 4 lines
[1]: makedumpfile, commit e73016540293 ("[v1.6.7] Update version")
Signed-off-by Pingfan Liu <[email protected]>
Quoted 10 lines
To: [email protected]
Cc: Andrew Morton <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Oscar Salvador <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: Qian Cai <[email protected]>
Cc: [email protected]
Cc: Kazuhito Hagio <[email protected]>

I think you dropped my

Acked-by David Hildenbrand <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

Pingfan Liu

author

2020-01-20 10:13 UTC #3 lore raw

Quoted 1 line · David Hildenbrand

[...]

Quoted 4 lines

I think you dropped my
Acked-by David Hildenbrand <[email protected]>

Sorry to forget it.
Thanks for your kind review.

Regards,
Pingfan

Andrew Morton

maintainer

2020-01-24 03:10 UTC #4 lore raw

In reply to Michal Hocko

Quoted 14 lines · On Mon, 20 Jan 2020 08:29:39 +0100 Michal Hocko <[email protected]> wrote:

Quoted 13 lines · Pingfan Liu
After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"), when a mem section is fully deactivated, section_mem_map still records the section's start pfn, which is not used any more and will be reassigned during re-added.
In analogy with alloc/free pattern, it is better to clear all fields of section_mem_map.
Beside this, it breaks the user space tool "makedumpfile" [1], which makes assumption that a hot-removed section has mem_map as NULL, instead of checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be better to change the assumption, and need a patch)

Quoted 6 lines

Quoted 2 lines
The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" , trigger a crash, and save vmcore by makedumpfile
While makedumpfile lives very closely to the kernel and occasional breakage is to be expected I still believe that Fixes: ba72b4c8cf60 is due.

But not a cc:stable?

Michal Hocko

reviewer

2020-01-24 06:49 UTC #5 lore raw

Quoted 16 lines · Andrew Morton

Quoted 14 lines · On Mon, 20 Jan 2020 08:29:39 +0100 Michal Hocko <[email protected]> wrote:
Quoted 13 lines · Pingfan Liu
After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"), when a mem section is fully deactivated, section_mem_map still records the section's start pfn, which is not used any more and will be reassigned during re-added.
In analogy with alloc/free pattern, it is better to clear all fields of section_mem_map.
Beside this, it breaks the user space tool "makedumpfile" [1], which makes assumption that a hot-removed section has mem_map as NULL, instead of checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be better to change the assumption, and need a patch)

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" , trigger a crash, and save vmcore by makedumpfile
While makedumpfile lives very closely to the kernel and occasional breakage is to be expected I still believe that Fixes: ba72b4c8cf60 is due.
But not a cc:stable?

Well, I wouldn't say this is really critical. makedumpfile will get its fix... But if people think it would be useful in stable I won't oppose.

Signature · 2 lines

Michal Hocko
SUSE Labs

Pingfan Liu

author

2020-01-25 13:27 UTC #6 lore raw

Quoted 21 lines · Michal Hocko

Quoted 19 lines · Andrew Morton
Quoted 17 lines · On Mon, 20 Jan 2020 08:29:39 +0100 Michal Hocko <[email protected]> wrote:
Quoted 15 lines · Pingfan Liu
After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"), when a mem section is fully deactivated, section_mem_map still records the section's start pfn, which is not used any more and will be reassigned during re-added.
In analogy with alloc/free pattern, it is better to clear all fields of section_mem_map.
Beside this, it breaks the user space tool "makedumpfile" [1], which makes assumption that a hot-removed section has mem_map as NULL, instead of checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be better to change the assumption, and need a patch)
The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" , trigger a crash, and save vmcore by makedumpfile

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
While makedumpfile lives very closely to the kernel and occasional breakage is to be expected I still believe that Fixes: ba72b4c8cf60 is due.
But not a cc:stable?
Well, I wouldn't say this is really critical. makedumpfile will get its fix... But if people think it would be useful in stable I won't oppose.

Yes, I think this patch is no more than a prototype improvement, and makedumpfile has better to get its fix.
And I have sent a patch to kexec-list for it.
(http://lists.infradead.org/pipermail/kexec/2020-January/024406.html)

Thanks,
Pingfan