| Message ID | 20200307084229.28251-4-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hotplug: Only use subsection map for VMEMMAP | expand |
On 07.03.20 09:42, Baoquan He wrote: > Factor out the code which clear subsection map of one memory region from > section_deactivate() into clear_subsection_map(). > > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > mm/sparse.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/mm/sparse.c b/mm/sparse.c > index e37c0abcdc89..d9dcd58d5c1d 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c > @@ -726,15 +726,11 @@ static void free_map_bootmem(struct page *memmap) > } > #endif /* CONFIG_SPARSEMEM_VMEMMAP */ > > -static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > - struct vmem_altmap *altmap) > +static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > { > DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; > DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 }; > struct mem_section *ms = __pfn_to_section(pfn); > - bool section_is_early = early_section(ms); > - struct page *memmap = NULL; > - bool empty = false; > unsigned long *subsection_map = ms->usage > ? &ms->usage->subsection_map[0] : NULL; > > @@ -745,8 +741,31 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION), > "section already deactivated (%#lx + %ld)\n", > pfn, nr_pages)) > + return -EINVAL; > + > + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > + Nit: I'd drop this line. > + return 0; > +} > + > +static bool is_subsection_map_empty(struct mem_section *ms) > +{ > + return bitmap_empty(&ms->usage->subsection_map[0], > + SUBSECTIONS_PER_SECTION); > +} > + > +static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > + struct vmem_altmap *altmap) > +{ > + struct mem_section *ms = __pfn_to_section(pfn); > + bool section_is_early = early_section(ms); > + struct page *memmap = NULL; > + bool empty = false; Nit: No need to initialize empty. > + > + if (clear_subsection_map(pfn, nr_pages)) > return; > Nit: I'd drop this empty line. > + empty = is_subsection_map_empty(ms); > /* > * There are 3 cases to handle across two configurations > * (SPARSEMEM_VMEMMAP={y,n}): > @@ -764,8 +783,6 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > * > * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified > */ > - bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > - empty = bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); I do wonder why you moved this up the comment? > if (empty) { > unsigned long section_nr = pfn_to_section_nr(pfn); > > Reviewed-by: David Hildenbrand <david@redhat.com>
On 03/09/20 at 09:59am, David Hildenbrand wrote: > On 07.03.20 09:42, Baoquan He wrote: > > Factor out the code which clear subsection map of one memory region from > > section_deactivate() into clear_subsection_map(). > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > --- > > mm/sparse.c | 31 ++++++++++++++++++++++++------- > > 1 file changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/mm/sparse.c b/mm/sparse.c > > index e37c0abcdc89..d9dcd58d5c1d 100644 > > --- a/mm/sparse.c > > +++ b/mm/sparse.c > > @@ -726,15 +726,11 @@ static void free_map_bootmem(struct page *memmap) > > } > > #endif /* CONFIG_SPARSEMEM_VMEMMAP */ > > > > -static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > > - struct vmem_altmap *altmap) > > +static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > > { > > DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; > > DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 }; > > struct mem_section *ms = __pfn_to_section(pfn); > > - bool section_is_early = early_section(ms); > > - struct page *memmap = NULL; > > - bool empty = false; > > unsigned long *subsection_map = ms->usage > > ? &ms->usage->subsection_map[0] : NULL; > > > > @@ -745,8 +741,31 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > > if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION), > > "section already deactivated (%#lx + %ld)\n", > > pfn, nr_pages)) > > + return -EINVAL; > > + > > + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > > + > > Nit: I'd drop this line. It's fine to me. I usually keep one line for the returning. I will remove it when update. > > > + return 0; > > +} > > + > > +static bool is_subsection_map_empty(struct mem_section *ms) > > +{ > > + return bitmap_empty(&ms->usage->subsection_map[0], > > + SUBSECTIONS_PER_SECTION); > > +} > > + > > +static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > > + struct vmem_altmap *altmap) > > +{ > > + struct mem_section *ms = __pfn_to_section(pfn); > > + bool section_is_early = early_section(ms); > > + struct page *memmap = NULL; > > + bool empty = false; > > Nit: No need to initialize empty. This is inherited from patch 1. > > > + > > + if (clear_subsection_map(pfn, nr_pages)) > > return; > > > > Nit: I'd drop this empty line. > > > + empty = is_subsection_map_empty(ms); > > /* > > * There are 3 cases to handle across two configurations > > * (SPARSEMEM_VMEMMAP={y,n}): > > @@ -764,8 +783,6 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > > * > > * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified > > */ > > - bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > > - empty = bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); > > I do wonder why you moved this up the comment? Since this empty will cover two places of handling, so moved it up, seems this is what I was thinking. Can move it back here. > > > if (empty) { > > unsigned long section_nr = pfn_to_section_nr(pfn); > > > > > > Reviewed-by: David Hildenbrand <david@redhat.com> > > -- > Thanks, > > David / dhildenb
On 09.03.20 14:32, Baoquan He wrote: > On 03/09/20 at 09:59am, David Hildenbrand wrote: >> On 07.03.20 09:42, Baoquan He wrote: >>> Factor out the code which clear subsection map of one memory region from >>> section_deactivate() into clear_subsection_map(). >>> >>> Signed-off-by: Baoquan He <bhe@redhat.com> >>> --- >>> mm/sparse.c | 31 ++++++++++++++++++++++++------- >>> 1 file changed, 24 insertions(+), 7 deletions(-) >>> >>> diff --git a/mm/sparse.c b/mm/sparse.c >>> index e37c0abcdc89..d9dcd58d5c1d 100644 >>> --- a/mm/sparse.c >>> +++ b/mm/sparse.c >>> @@ -726,15 +726,11 @@ static void free_map_bootmem(struct page *memmap) >>> } >>> #endif /* CONFIG_SPARSEMEM_VMEMMAP */ >>> >>> -static void section_deactivate(unsigned long pfn, unsigned long nr_pages, >>> - struct vmem_altmap *altmap) >>> +static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) >>> { >>> DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; >>> DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 }; >>> struct mem_section *ms = __pfn_to_section(pfn); >>> - bool section_is_early = early_section(ms); >>> - struct page *memmap = NULL; >>> - bool empty = false; >>> unsigned long *subsection_map = ms->usage >>> ? &ms->usage->subsection_map[0] : NULL; >>> >>> @@ -745,8 +741,31 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, >>> if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION), >>> "section already deactivated (%#lx + %ld)\n", >>> pfn, nr_pages)) >>> + return -EINVAL; >>> + >>> + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); >>> + >> >> Nit: I'd drop this line. > > It's fine to me. I usually keep one line for the returning. I will > remove it when update. > >> >>> + return 0; >>> +} >>> + >>> +static bool is_subsection_map_empty(struct mem_section *ms) >>> +{ >>> + return bitmap_empty(&ms->usage->subsection_map[0], >>> + SUBSECTIONS_PER_SECTION); >>> +} >>> + >>> +static void section_deactivate(unsigned long pfn, unsigned long nr_pages, >>> + struct vmem_altmap *altmap) >>> +{ >>> + struct mem_section *ms = __pfn_to_section(pfn); >>> + bool section_is_early = early_section(ms); >>> + struct page *memmap = NULL; >>> + bool empty = false; >> >> Nit: No need to initialize empty. > > This is inherited from patch 1. > >> >>> + >>> + if (clear_subsection_map(pfn, nr_pages)) >>> return; >>> >> >> Nit: I'd drop this empty line. >> >>> + empty = is_subsection_map_empty(ms); >>> /* >>> * There are 3 cases to handle across two configurations >>> * (SPARSEMEM_VMEMMAP={y,n}): >>> @@ -764,8 +783,6 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, >>> * >>> * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified >>> */ >>> - bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); >>> - empty = bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); >> >> I do wonder why you moved this up the comment? > > Since this empty will cover two places of handling, so moved it up, > seems this is what I was thinking. Can move it back here. You're moving the whole comment later, was just wondering (makes it slightly harder to review).
On 03/09/20 at 02:38pm, David Hildenbrand wrote: > On 09.03.20 14:32, Baoquan He wrote: > > On 03/09/20 at 09:59am, David Hildenbrand wrote: > >> On 07.03.20 09:42, Baoquan He wrote: > >>> Factor out the code which clear subsection map of one memory region from > >>> section_deactivate() into clear_subsection_map(). > >>> > >>> Signed-off-by: Baoquan He <bhe@redhat.com> > >>> --- > >>> mm/sparse.c | 31 ++++++++++++++++++++++++------- > >>> 1 file changed, 24 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/mm/sparse.c b/mm/sparse.c > >>> index e37c0abcdc89..d9dcd58d5c1d 100644 > >>> --- a/mm/sparse.c > >>> +++ b/mm/sparse.c > >>> @@ -726,15 +726,11 @@ static void free_map_bootmem(struct page *memmap) > >>> } > >>> #endif /* CONFIG_SPARSEMEM_VMEMMAP */ > >>> > >>> -static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > >>> - struct vmem_altmap *altmap) > >>> +static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > >>> { > >>> DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; > >>> DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 }; > >>> struct mem_section *ms = __pfn_to_section(pfn); > >>> - bool section_is_early = early_section(ms); > >>> - struct page *memmap = NULL; > >>> - bool empty = false; > >>> unsigned long *subsection_map = ms->usage > >>> ? &ms->usage->subsection_map[0] : NULL; > >>> > >>> @@ -745,8 +741,31 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > >>> if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION), > >>> "section already deactivated (%#lx + %ld)\n", > >>> pfn, nr_pages)) > >>> + return -EINVAL; > >>> + > >>> + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > >>> + > >> > >> Nit: I'd drop this line. > > > > It's fine to me. I usually keep one line for the returning. I will > > remove it when update. > > > >> > >>> + return 0; > >>> +} > >>> + > >>> +static bool is_subsection_map_empty(struct mem_section *ms) > >>> +{ > >>> + return bitmap_empty(&ms->usage->subsection_map[0], > >>> + SUBSECTIONS_PER_SECTION); > >>> +} > >>> + > >>> +static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > >>> + struct vmem_altmap *altmap) > >>> +{ > >>> + struct mem_section *ms = __pfn_to_section(pfn); > >>> + bool section_is_early = early_section(ms); > >>> + struct page *memmap = NULL; > >>> + bool empty = false; > >> > >> Nit: No need to initialize empty. > > > > This is inherited from patch 1. > > > >> > >>> + > >>> + if (clear_subsection_map(pfn, nr_pages)) > >>> return; > >>> > >> > >> Nit: I'd drop this empty line. > >> > >>> + empty = is_subsection_map_empty(ms); > >>> /* > >>> * There are 3 cases to handle across two configurations > >>> * (SPARSEMEM_VMEMMAP={y,n}): > >>> @@ -764,8 +783,6 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > >>> * > >>> * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified > >>> */ > >>> - bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > >>> - empty = bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); > >> > >> I do wonder why you moved this up the comment? > > > > Since this empty will cover two places of handling, so moved it up, > > seems this is what I was thinking. Can move it back here. > > You're moving the whole comment later, was just wondering (makes it > slightly harder to review). I see, sorry for the confusion. I will move it back when repost.
diff --git a/mm/sparse.c b/mm/sparse.c index e37c0abcdc89..d9dcd58d5c1d 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -726,15 +726,11 @@ static void free_map_bootmem(struct page *memmap) } #endif /* CONFIG_SPARSEMEM_VMEMMAP */ -static void section_deactivate(unsigned long pfn, unsigned long nr_pages, - struct vmem_altmap *altmap) +static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) { DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; DECLARE_BITMAP(tmp, SUBSECTIONS_PER_SECTION) = { 0 }; struct mem_section *ms = __pfn_to_section(pfn); - bool section_is_early = early_section(ms); - struct page *memmap = NULL; - bool empty = false; unsigned long *subsection_map = ms->usage ? &ms->usage->subsection_map[0] : NULL; @@ -745,8 +741,31 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION), "section already deactivated (%#lx + %ld)\n", pfn, nr_pages)) + return -EINVAL; + + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); + + return 0; +} + +static bool is_subsection_map_empty(struct mem_section *ms) +{ + return bitmap_empty(&ms->usage->subsection_map[0], + SUBSECTIONS_PER_SECTION); +} + +static void section_deactivate(unsigned long pfn, unsigned long nr_pages, + struct vmem_altmap *altmap) +{ + struct mem_section *ms = __pfn_to_section(pfn); + bool section_is_early = early_section(ms); + struct page *memmap = NULL; + bool empty = false; + + if (clear_subsection_map(pfn, nr_pages)) return; + empty = is_subsection_map_empty(ms); /* * There are 3 cases to handle across two configurations * (SPARSEMEM_VMEMMAP={y,n}): @@ -764,8 +783,6 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, * * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified */ - bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); - empty = bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); if (empty) { unsigned long section_nr = pfn_to_section_nr(pfn);
Factor out the code which clear subsection map of one memory region from section_deactivate() into clear_subsection_map(). Signed-off-by: Baoquan He <bhe@redhat.com> --- mm/sparse.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)