| Message ID | 1579487594-28889-1-git-send-email-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2] mm/sparse: reset section's mem_map when fully deactivated | expand |
On Mon 20-01-20 10:33:14, Pingfan Liu wrote: > 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 While makedumpfile lives very closely to the kernel and occasional breakage is to be expected I still believe that Fixes: ba72b4c8cf60 is due. > [1]: makedumpfile, commit e73016540293 ("[v1.6.7] Update version") > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > To: linux-mm@kvack.org > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: David Hildenbrand <david@redhat.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Michal Hocko <mhocko@kernel.org> > Cc: Baoquan He <bhe@redhat.com> > Cc: Qian Cai <cai@lca.pw> > Cc: kexec@lists.infradead.org > Cc: Kazuhito Hagio <k-hagio@ab.jp.nec.com> Acked-by: Michal Hocko <mhocko@suse.com> > --- > v1 -> v2: > make an explicit convertion from NULL to ulong > improve commit log > > mm/sparse.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/sparse.c b/mm/sparse.c > index 3822ecb..3918fc3 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c > @@ -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) > -- > 2.7.5
On 20.01.20 08:29, Michal Hocko wrote: > On Mon 20-01-20 10:33:14, Pingfan Liu wrote: >> 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 > > 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. > >> [1]: makedumpfile, commit e73016540293 ("[v1.6.7] Update version") >> >> Signed-off-by: Pingfan Liu <kernelfans@gmail.com> >> To: linux-mm@kvack.org >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: David Hildenbrand <david@redhat.com> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: Oscar Salvador <osalvador@suse.de> >> Cc: Michal Hocko <mhocko@kernel.org> >> Cc: Baoquan He <bhe@redhat.com> >> Cc: Qian Cai <cai@lca.pw> >> Cc: kexec@lists.infradead.org >> Cc: Kazuhito Hagio <k-hagio@ab.jp.nec.com> > I think you dropped my Acked-by: David Hildenbrand <david@redhat.com>
On Mon, Jan 20, 2020 at 5:03 PM David Hildenbrand <david@redhat.com> wrote: > [...] > I think you dropped my > > Acked-by: David Hildenbrand <david@redhat.com> > Sorry to forget it. Thanks for your kind review. Regards, Pingfan
On Mon, 20 Jan 2020 08:29:39 +0100 Michal Hocko <mhocko@suse.com> wrote: > On Mon 20-01-20 10:33:14, Pingfan Liu wrote: > > 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 > > 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?
On Thu 23-01-20 19:10:47, Andrew Morton wrote: > On Mon, 20 Jan 2020 08:29:39 +0100 Michal Hocko <mhocko@suse.com> wrote: > > > On Mon 20-01-20 10:33:14, Pingfan Liu wrote: > > > 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 > > > > 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.
On Fri, Jan 24, 2020 at 2:49 PM Michal Hocko <mhocko@suse.com> wrote: > > On Thu 23-01-20 19:10:47, Andrew Morton wrote: > > On Mon, 20 Jan 2020 08:29:39 +0100 Michal Hocko <mhocko@suse.com> wrote: > > > > > On Mon 20-01-20 10:33:14, Pingfan Liu wrote: > > > > 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 > > > > > > 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
diff --git a/mm/sparse.c b/mm/sparse.c index 3822ecb..3918fc3 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -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)
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 <kernelfans@gmail.com> To: linux-mm@kvack.org Cc: Andrew Morton <akpm@linux-foundation.org> Cc: David Hildenbrand <david@redhat.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Oscar Salvador <osalvador@suse.de> Cc: Michal Hocko <mhocko@kernel.org> Cc: Baoquan He <bhe@redhat.com> Cc: Qian Cai <cai@lca.pw> Cc: kexec@lists.infradead.org Cc: Kazuhito Hagio <k-hagio@ab.jp.nec.com> --- v1 -> v2: make an explicit convertion from NULL to ulong improve commit log mm/sparse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)