| Message ID | 20200220043316.19668-4-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hotplug: Only use subsection map in VMEMMAP case | expand |
On Thu, Feb 20, 2020 at 12:33:12PM +0800, Baoquan He wrote: >Wrap the codes which clear subsection map of one memory region from >section_deactivate() into clear_subsection_map(). > >Signed-off-by: Baoquan He <bhe@redhat.com> Reviewed-by: Wei Yang <richardw.yang@linux.intel.com> >--- > mm/sparse.c | 46 ++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 38 insertions(+), 8 deletions(-) > >diff --git a/mm/sparse.c b/mm/sparse.c >index 977b47acd38d..df857ee9330c 100644 >--- a/mm/sparse.c >+++ b/mm/sparse.c >@@ -726,14 +726,25 @@ 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) >+/** >+ * clear_subsection_map - Clear subsection map of one memory region >+ * >+ * @pfn - start pfn of the memory range >+ * @nr_pages - number of pfns to add in the region >+ * >+ * This is only intended for hotplug, and clear the related subsection >+ * map inside one section. >+ * >+ * Return: >+ * * -EINVAL - Section already deactived. >+ * * 0 - Subsection map is emptied. >+ * * 1 - Subsection map is not empty. >+ */ >+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; > unsigned long *subsection_map = ms->usage > ? &ms->usage->subsection_map[0] : NULL; > >@@ -744,8 +755,28 @@ 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; >+ return -EINVAL; >+ >+ bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > >+ if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) >+ return 0; >+ >+ return 1; >+} >+ >+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; >+ int rc; >+ >+ >+ rc = clear_subsection_map(pfn, nr_pages); >+ if (IS_ERR_VALUE((unsigned long)rc)) >+ return; > /* > * There are 3 cases to handle across two configurations > * (SPARSEMEM_VMEMMAP={y,n}): >@@ -763,8 +794,7 @@ 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); >- if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) { >+ if (!rc) { > unsigned long section_nr = pfn_to_section_nr(pfn); > > /* >@@ -786,7 +816,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > else > depopulate_section_memmap(pfn, nr_pages, altmap); > >- if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) >+ if (!rc) > ms->section_mem_map = (unsigned long)NULL; > } > >-- >2.17.2
On 20.02.20 05:33, Baoquan He wrote: > Wrap the codes 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 | 46 ++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 38 insertions(+), 8 deletions(-) > > diff --git a/mm/sparse.c b/mm/sparse.c > index 977b47acd38d..df857ee9330c 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c > @@ -726,14 +726,25 @@ 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) > +/** > + * clear_subsection_map - Clear subsection map of one memory region > + * > + * @pfn - start pfn of the memory range > + * @nr_pages - number of pfns to add in the region > + * > + * This is only intended for hotplug, and clear the related subsection > + * map inside one section. > + * > + * Return: > + * * -EINVAL - Section already deactived. > + * * 0 - Subsection map is emptied. > + * * 1 - Subsection map is not empty. > + */ Less verbose please (in my preference: none and simplify return handling) > +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; > unsigned long *subsection_map = ms->usage > ? &ms->usage->subsection_map[0] : NULL; > > @@ -744,8 +755,28 @@ 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; > + return -EINVAL; > + > + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > > + if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > + return 0; > + Can we please just have a subsection_map_empty() instead and handle that in the caller? (you can then always return true in the !VMEMMAP variant) I dislike the "rc" handling in the caller. > + return 1; > +} > + > +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; > + int rc; > + > + one superfluous empty line > + rc = clear_subsection_map(pfn, nr_pages); > + if (IS_ERR_VALUE((unsigned long)rc)) huh? "if (rc < 0)" ? or am I missing something? > + return; > /* > * There are 3 cases to handle across two configurations > * (SPARSEMEM_VMEMMAP={y,n}): > @@ -763,8 +794,7 @@ 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); > - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) { > + if (!rc) { > unsigned long section_nr = pfn_to_section_nr(pfn); > > /* > @@ -786,7 +816,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > else > depopulate_section_memmap(pfn, nr_pages, altmap); > > - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > + if (!rc) I don't really like that handling. Either s/rc/section_empty/ or use a separate subsection_map_empty() > ms->section_mem_map = (unsigned long)NULL; > } > > Thanks!
On 02/28/20 at 03:36pm, David Hildenbrand wrote: > On 20.02.20 05:33, Baoquan He wrote: > > Wrap the codes 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 | 46 ++++++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 38 insertions(+), 8 deletions(-) > > > > diff --git a/mm/sparse.c b/mm/sparse.c > > index 977b47acd38d..df857ee9330c 100644 > > --- a/mm/sparse.c > > +++ b/mm/sparse.c > > @@ -726,14 +726,25 @@ 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) > > +/** > > + * clear_subsection_map - Clear subsection map of one memory region > > + * > > + * @pfn - start pfn of the memory range > > + * @nr_pages - number of pfns to add in the region > > + * > > + * This is only intended for hotplug, and clear the related subsection > > + * map inside one section. > > + * > > + * Return: > > + * * -EINVAL - Section already deactived. > > + * * 0 - Subsection map is emptied. > > + * * 1 - Subsection map is not empty. > > + */ > > Less verbose please (in my preference: none and simplify return handling) > > > +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; > > unsigned long *subsection_map = ms->usage > > ? &ms->usage->subsection_map[0] : NULL; > > > > @@ -744,8 +755,28 @@ 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; > > + return -EINVAL; > > + > > + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > > > > + if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > > + return 0; > > + > > Can we please just have a > > subsection_map_empty() instead and handle that in the caller? > (you can then always return true in the !VMEMMAP variant) I don't follow. Could you be more specific? or pseudo code please? The old code has to handle below case in which subsection_map has been cleared. And I introduce clear_subsection_map() to encapsulate all subsection map realted code so that !VMEMMAP won't have to see it any more. if (WARN(!subsection_map || !bitmap_equal(tmp, map, SUBSECTIONS_PER_SECTION), "section already deactivated (%#lx + %ld)\n", pfn, nr_pages)) return; > > I dislike the "rc" handling in the caller. > > > + return 1; > > +} > > + > > +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; > > + int rc; > > + > > + > > one superfluous empty line > > > + rc = clear_subsection_map(pfn, nr_pages); > > + if (IS_ERR_VALUE((unsigned long)rc)) > > huh? "if (rc < 0)" ? or am I missing something? Both is fine to me, I have no preference, can change like this. > > > + return; > > /* > > * There are 3 cases to handle across two configurations > > * (SPARSEMEM_VMEMMAP={y,n}): > > @@ -763,8 +794,7 @@ 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); > > - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) { > > + if (!rc) { > > unsigned long section_nr = pfn_to_section_nr(pfn); > > > > /* > > @@ -786,7 +816,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > > else > > depopulate_section_memmap(pfn, nr_pages, altmap); > > > > - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > > + if (!rc) > > I don't really like that handling. > > Either > > s/rc/section_empty/ > > or use a separate subsection_map_empty() > > > ms->section_mem_map = (unsigned long)NULL; > > } > > > > > > Thanks! > > -- > Thanks, > > David / dhildenb
On 01.03.20 06:20, Baoquan He wrote: > On 02/28/20 at 03:36pm, David Hildenbrand wrote: >> On 20.02.20 05:33, Baoquan He wrote: >>> Wrap the codes 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 | 46 ++++++++++++++++++++++++++++++++++++++-------- >>> 1 file changed, 38 insertions(+), 8 deletions(-) >>> >>> diff --git a/mm/sparse.c b/mm/sparse.c >>> index 977b47acd38d..df857ee9330c 100644 >>> --- a/mm/sparse.c >>> +++ b/mm/sparse.c >>> @@ -726,14 +726,25 @@ 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) >>> +/** >>> + * clear_subsection_map - Clear subsection map of one memory region >>> + * >>> + * @pfn - start pfn of the memory range >>> + * @nr_pages - number of pfns to add in the region >>> + * >>> + * This is only intended for hotplug, and clear the related subsection >>> + * map inside one section. >>> + * >>> + * Return: >>> + * * -EINVAL - Section already deactived. >>> + * * 0 - Subsection map is emptied. >>> + * * 1 - Subsection map is not empty. >>> + */ >> >> Less verbose please (in my preference: none and simplify return handling) >> >>> +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; >>> unsigned long *subsection_map = ms->usage >>> ? &ms->usage->subsection_map[0] : NULL; >>> >>> @@ -744,8 +755,28 @@ 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; >>> + return -EINVAL; >>> + >>> + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); >>> >>> + if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) >>> + return 0; >>> + >> >> Can we please just have a >> >> subsection_map_empty() instead and handle that in the caller? >> (you can then always return true in the !VMEMMAP variant) > > I don't follow. Could you be more specific? or pseudo code please? > > The old code has to handle below case in which subsection_map has been > cleared. And I introduce clear_subsection_map() to encapsulate all > subsection map realted code so that !VMEMMAP won't have to see it any > more. > Something like this on top would be easier to understand IMHO diff --git a/mm/sparse.c b/mm/sparse.c index dc79b00ddaaa..be5c80e9cfee 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -726,20 +726,6 @@ static void free_map_bootmem(struct page *memmap) } #endif /* CONFIG_SPARSEMEM_VMEMMAP */ -/** - * clear_subsection_map - Clear subsection map of one memory region - * - * @pfn - start pfn of the memory range - * @nr_pages - number of pfns to add in the region - * - * This is only intended for hotplug, and clear the related subsection - * map inside one section. - * - * Return: - * * -EINVAL - Section already deactived. - * * 0 - Subsection map is emptied. - * * 1 - Subsection map is not empty. - */ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) { DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; @@ -758,11 +744,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) return -EINVAL; bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); + return 0; +} - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) - return 0; - - return 1; +static bool is_subsection_map_empty(unsigned long pfn, unsigned long nr_pages) +{ + return bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); } static void section_deactivate(unsigned long pfn, unsigned long nr_pages, @@ -771,11 +758,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, struct mem_section *ms = __pfn_to_section(pfn); bool section_is_early = early_section(ms); struct page *memmap = NULL; - int rc; - - rc = clear_subsection_map(pfn, nr_pages); - if (IS_ERR_VALUE((unsigned long)rc)) + if (unlikely(clear_subsection_map(pfn, nr_pages))) return; /* * There are 3 cases to handle across two configurations @@ -794,7 +778,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, * * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified */ - if (!rc) { + if (is_subsection_map_empty(pfn, nr_pages)) { unsigned long section_nr = pfn_to_section_nr(pfn); /* @@ -816,7 +800,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, else depopulate_section_memmap(pfn, nr_pages, altmap); - if (!rc) + if (is_subsection_map_empty(pfn, nr_pages)) ms->section_mem_map = (unsigned long)NULL; }
On 03/02/20 at 04:43pm, David Hildenbrand wrote: > On 01.03.20 06:20, Baoquan He wrote: > > On 02/28/20 at 03:36pm, David Hildenbrand wrote: > >> On 20.02.20 05:33, Baoquan He wrote: > >>> Wrap the codes 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 | 46 ++++++++++++++++++++++++++++++++++++++-------- > >>> 1 file changed, 38 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/mm/sparse.c b/mm/sparse.c > >>> index 977b47acd38d..df857ee9330c 100644 > >>> --- a/mm/sparse.c > >>> +++ b/mm/sparse.c > >>> @@ -726,14 +726,25 @@ 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) > >>> +/** > >>> + * clear_subsection_map - Clear subsection map of one memory region > >>> + * > >>> + * @pfn - start pfn of the memory range > >>> + * @nr_pages - number of pfns to add in the region > >>> + * > >>> + * This is only intended for hotplug, and clear the related subsection > >>> + * map inside one section. > >>> + * > >>> + * Return: > >>> + * * -EINVAL - Section already deactived. > >>> + * * 0 - Subsection map is emptied. > >>> + * * 1 - Subsection map is not empty. > >>> + */ > >> > >> Less verbose please (in my preference: none and simplify return handling) > >> > >>> +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; > >>> unsigned long *subsection_map = ms->usage > >>> ? &ms->usage->subsection_map[0] : NULL; > >>> > >>> @@ -744,8 +755,28 @@ 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; > >>> + return -EINVAL; > >>> + > >>> + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > >>> > >>> + if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > >>> + return 0; > >>> + > >> > >> Can we please just have a > >> > >> subsection_map_empty() instead and handle that in the caller? > >> (you can then always return true in the !VMEMMAP variant) > > > > I don't follow. Could you be more specific? or pseudo code please? > > > > The old code has to handle below case in which subsection_map has been > > cleared. And I introduce clear_subsection_map() to encapsulate all > > subsection map realted code so that !VMEMMAP won't have to see it any > > more. > > > > Something like this on top would be easier to understand IMHO Ok, I see. Both is fine to me, I even like the old way a tiny little bit more, but I would like to try to satisfy reviewer, will change as you suggested. Thanks. If no other concern, I will repost after changing and testing. > > > diff --git a/mm/sparse.c b/mm/sparse.c > index dc79b00ddaaa..be5c80e9cfee 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c > @@ -726,20 +726,6 @@ static void free_map_bootmem(struct page *memmap) > } > #endif /* CONFIG_SPARSEMEM_VMEMMAP */ > > -/** > - * clear_subsection_map - Clear subsection map of one memory region > - * > - * @pfn - start pfn of the memory range > - * @nr_pages - number of pfns to add in the region > - * > - * This is only intended for hotplug, and clear the related subsection > - * map inside one section. > - * > - * Return: > - * * -EINVAL - Section already deactived. > - * * 0 - Subsection map is emptied. > - * * 1 - Subsection map is not empty. > - */ > static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > { > DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; > @@ -758,11 +744,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > return -EINVAL; > > bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > + return 0; > +} > > - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > - return 0; > - > - return 1; > +static bool is_subsection_map_empty(unsigned long pfn, unsigned long nr_pages) > +{ > + return bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); > } > > static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > @@ -771,11 +758,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > struct mem_section *ms = __pfn_to_section(pfn); > bool section_is_early = early_section(ms); > struct page *memmap = NULL; > - int rc; > - > > - rc = clear_subsection_map(pfn, nr_pages); > - if (IS_ERR_VALUE((unsigned long)rc)) > + if (unlikely(clear_subsection_map(pfn, nr_pages))) > return; > /* > * There are 3 cases to handle across two configurations > @@ -794,7 +778,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > * > * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified > */ > - if (!rc) { > + if (is_subsection_map_empty(pfn, nr_pages)) { > unsigned long section_nr = pfn_to_section_nr(pfn); > > /* > @@ -816,7 +800,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > else > depopulate_section_memmap(pfn, nr_pages, altmap); > > - if (!rc) > + if (is_subsection_map_empty(pfn, nr_pages)) > ms->section_mem_map = (unsigned long)NULL; > } > > > -- > Thanks, > > David / dhildenb
On 03/02/20 at 04:43pm, David Hildenbrand wrote: > On 01.03.20 06:20, Baoquan He wrote: > > On 02/28/20 at 03:36pm, David Hildenbrand wrote: > >> On 20.02.20 05:33, Baoquan He wrote: > >>> Wrap the codes 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 | 46 ++++++++++++++++++++++++++++++++++++++-------- > >>> 1 file changed, 38 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/mm/sparse.c b/mm/sparse.c > >>> index 977b47acd38d..df857ee9330c 100644 > >>> --- a/mm/sparse.c > >>> +++ b/mm/sparse.c > >>> @@ -726,14 +726,25 @@ 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) > >>> +/** > >>> + * clear_subsection_map - Clear subsection map of one memory region > >>> + * > >>> + * @pfn - start pfn of the memory range > >>> + * @nr_pages - number of pfns to add in the region > >>> + * > >>> + * This is only intended for hotplug, and clear the related subsection > >>> + * map inside one section. > >>> + * > >>> + * Return: > >>> + * * -EINVAL - Section already deactived. > >>> + * * 0 - Subsection map is emptied. > >>> + * * 1 - Subsection map is not empty. > >>> + */ > >> > >> Less verbose please (in my preference: none and simplify return handling) > >> > >>> +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; > >>> unsigned long *subsection_map = ms->usage > >>> ? &ms->usage->subsection_map[0] : NULL; > >>> > >>> @@ -744,8 +755,28 @@ 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; > >>> + return -EINVAL; > >>> + > >>> + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > >>> > >>> + if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > >>> + return 0; > >>> + > >> > >> Can we please just have a > >> > >> subsection_map_empty() instead and handle that in the caller? > >> (you can then always return true in the !VMEMMAP variant) > > > > I don't follow. Could you be more specific? or pseudo code please? > > > > The old code has to handle below case in which subsection_map has been > > cleared. And I introduce clear_subsection_map() to encapsulate all > > subsection map realted code so that !VMEMMAP won't have to see it any > > more. > > > > Something like this on top would be easier to understand IMHO > > > diff --git a/mm/sparse.c b/mm/sparse.c > index dc79b00ddaaa..be5c80e9cfee 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c > @@ -726,20 +726,6 @@ static void free_map_bootmem(struct page *memmap) > } > #endif /* CONFIG_SPARSEMEM_VMEMMAP */ > > -/** > - * clear_subsection_map - Clear subsection map of one memory region > - * > - * @pfn - start pfn of the memory range > - * @nr_pages - number of pfns to add in the region > - * > - * This is only intended for hotplug, and clear the related subsection > - * map inside one section. > - * > - * Return: > - * * -EINVAL - Section already deactived. > - * * 0 - Subsection map is emptied. > - * * 1 - Subsection map is not empty. > - */ > static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > { > DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; > @@ -758,11 +744,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > return -EINVAL; > > bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > + return 0; > +} > > - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > - return 0; > - > - return 1; > +static bool is_subsection_map_empty(unsigned long pfn, unsigned long nr_pages) > +{ > + return bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); > } > > static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > @@ -771,11 +758,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > struct mem_section *ms = __pfn_to_section(pfn); > bool section_is_early = early_section(ms); > struct page *memmap = NULL; > - int rc; > - > > - rc = clear_subsection_map(pfn, nr_pages); > - if (IS_ERR_VALUE((unsigned long)rc)) > + if (unlikely(clear_subsection_map(pfn, nr_pages))) > return; > /* > * There are 3 cases to handle across two configurations > @@ -794,7 +778,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > * > * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified > */ > - if (!rc) { > + if (is_subsection_map_empty(pfn, nr_pages)) { > unsigned long section_nr = pfn_to_section_nr(pfn); Tried this way, it's not good in this patch. Since ms->usage might be freed in this place. if (!PageReserved(virt_to_page(ms->usage))) { kfree(ms->usage); ms->usage = NULL; } If have to introduce is_subsection_map_empty(), the code need be adjusted a little bit. It may need be done in another separate patch to adjust it. If you agree, I would like to keep this patch as is, later I can refactor this on top of this patchset, or if anyone else post patch to adjust it, I can help review. > > /* > @@ -816,7 +800,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > else > depopulate_section_memmap(pfn, nr_pages, altmap); > > - if (!rc) > + if (is_subsection_map_empty(pfn, nr_pages)) > ms->section_mem_map = (unsigned long)NULL; > } > > > -- > Thanks, > > David / dhildenb
On 03.03.20 09:22, Baoquan He wrote: > On 03/02/20 at 04:43pm, David Hildenbrand wrote: >> On 01.03.20 06:20, Baoquan He wrote: >>> On 02/28/20 at 03:36pm, David Hildenbrand wrote: >>>> On 20.02.20 05:33, Baoquan He wrote: >>>>> Wrap the codes 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 | 46 ++++++++++++++++++++++++++++++++++++++-------- >>>>> 1 file changed, 38 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/mm/sparse.c b/mm/sparse.c >>>>> index 977b47acd38d..df857ee9330c 100644 >>>>> --- a/mm/sparse.c >>>>> +++ b/mm/sparse.c >>>>> @@ -726,14 +726,25 @@ 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) >>>>> +/** >>>>> + * clear_subsection_map - Clear subsection map of one memory region >>>>> + * >>>>> + * @pfn - start pfn of the memory range >>>>> + * @nr_pages - number of pfns to add in the region >>>>> + * >>>>> + * This is only intended for hotplug, and clear the related subsection >>>>> + * map inside one section. >>>>> + * >>>>> + * Return: >>>>> + * * -EINVAL - Section already deactived. >>>>> + * * 0 - Subsection map is emptied. >>>>> + * * 1 - Subsection map is not empty. >>>>> + */ >>>> >>>> Less verbose please (in my preference: none and simplify return handling) >>>> >>>>> +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; >>>>> unsigned long *subsection_map = ms->usage >>>>> ? &ms->usage->subsection_map[0] : NULL; >>>>> >>>>> @@ -744,8 +755,28 @@ 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; >>>>> + return -EINVAL; >>>>> + >>>>> + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); >>>>> >>>>> + if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) >>>>> + return 0; >>>>> + >>>> >>>> Can we please just have a >>>> >>>> subsection_map_empty() instead and handle that in the caller? >>>> (you can then always return true in the !VMEMMAP variant) >>> >>> I don't follow. Could you be more specific? or pseudo code please? >>> >>> The old code has to handle below case in which subsection_map has been >>> cleared. And I introduce clear_subsection_map() to encapsulate all >>> subsection map realted code so that !VMEMMAP won't have to see it any >>> more. >>> >> >> Something like this on top would be easier to understand IMHO >> >> >> diff --git a/mm/sparse.c b/mm/sparse.c >> index dc79b00ddaaa..be5c80e9cfee 100644 >> --- a/mm/sparse.c >> +++ b/mm/sparse.c >> @@ -726,20 +726,6 @@ static void free_map_bootmem(struct page *memmap) >> } >> #endif /* CONFIG_SPARSEMEM_VMEMMAP */ >> >> -/** >> - * clear_subsection_map - Clear subsection map of one memory region >> - * >> - * @pfn - start pfn of the memory range >> - * @nr_pages - number of pfns to add in the region >> - * >> - * This is only intended for hotplug, and clear the related subsection >> - * map inside one section. >> - * >> - * Return: >> - * * -EINVAL - Section already deactived. >> - * * 0 - Subsection map is emptied. >> - * * 1 - Subsection map is not empty. >> - */ >> static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) >> { >> DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; >> @@ -758,11 +744,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) >> return -EINVAL; >> >> bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); >> + return 0; >> +} >> >> - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) >> - return 0; >> - >> - return 1; >> +static bool is_subsection_map_empty(unsigned long pfn, unsigned long nr_pages) >> +{ >> + return bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); >> } >> >> static void section_deactivate(unsigned long pfn, unsigned long nr_pages, >> @@ -771,11 +758,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, >> struct mem_section *ms = __pfn_to_section(pfn); >> bool section_is_early = early_section(ms); >> struct page *memmap = NULL; >> - int rc; >> - >> >> - rc = clear_subsection_map(pfn, nr_pages); >> - if (IS_ERR_VALUE((unsigned long)rc)) >> + if (unlikely(clear_subsection_map(pfn, nr_pages))) >> return; >> /* >> * There are 3 cases to handle across two configurations >> @@ -794,7 +778,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, >> * >> * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified >> */ >> - if (!rc) { >> + if (is_subsection_map_empty(pfn, nr_pages)) { >> unsigned long section_nr = pfn_to_section_nr(pfn); > > Tried this way, it's not good in this patch. Since ms->usage might be > freed in this place. > > if (!PageReserved(virt_to_page(ms->usage))) { > kfree(ms->usage); > ms->usage = NULL; > } So your patch #1 is already broken. Just cache the result in patch #1. bool empty; ... empty = bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); ... if (empty) { ... }
On 03/03/20 at 09:33am, David Hildenbrand wrote: > On 03.03.20 09:22, Baoquan He wrote: > > On 03/02/20 at 04:43pm, David Hildenbrand wrote: > >> On 01.03.20 06:20, Baoquan He wrote: > >>> On 02/28/20 at 03:36pm, David Hildenbrand wrote: > >>>> On 20.02.20 05:33, Baoquan He wrote: > >>>>> Wrap the codes 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 | 46 ++++++++++++++++++++++++++++++++++++++-------- > >>>>> 1 file changed, 38 insertions(+), 8 deletions(-) > >>>>> > >>>>> diff --git a/mm/sparse.c b/mm/sparse.c > >>>>> index 977b47acd38d..df857ee9330c 100644 > >>>>> --- a/mm/sparse.c > >>>>> +++ b/mm/sparse.c > >>>>> @@ -726,14 +726,25 @@ 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) > >>>>> +/** > >>>>> + * clear_subsection_map - Clear subsection map of one memory region > >>>>> + * > >>>>> + * @pfn - start pfn of the memory range > >>>>> + * @nr_pages - number of pfns to add in the region > >>>>> + * > >>>>> + * This is only intended for hotplug, and clear the related subsection > >>>>> + * map inside one section. > >>>>> + * > >>>>> + * Return: > >>>>> + * * -EINVAL - Section already deactived. > >>>>> + * * 0 - Subsection map is emptied. > >>>>> + * * 1 - Subsection map is not empty. > >>>>> + */ > >>>> > >>>> Less verbose please (in my preference: none and simplify return handling) > >>>> > >>>>> +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; > >>>>> unsigned long *subsection_map = ms->usage > >>>>> ? &ms->usage->subsection_map[0] : NULL; > >>>>> > >>>>> @@ -744,8 +755,28 @@ 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; > >>>>> + return -EINVAL; > >>>>> + > >>>>> + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > >>>>> > >>>>> + if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > >>>>> + return 0; > >>>>> + > >>>> > >>>> Can we please just have a > >>>> > >>>> subsection_map_empty() instead and handle that in the caller? > >>>> (you can then always return true in the !VMEMMAP variant) > >>> > >>> I don't follow. Could you be more specific? or pseudo code please? > >>> > >>> The old code has to handle below case in which subsection_map has been > >>> cleared. And I introduce clear_subsection_map() to encapsulate all > >>> subsection map realted code so that !VMEMMAP won't have to see it any > >>> more. > >>> > >> > >> Something like this on top would be easier to understand IMHO > >> > >> > >> diff --git a/mm/sparse.c b/mm/sparse.c > >> index dc79b00ddaaa..be5c80e9cfee 100644 > >> --- a/mm/sparse.c > >> +++ b/mm/sparse.c > >> @@ -726,20 +726,6 @@ static void free_map_bootmem(struct page *memmap) > >> } > >> #endif /* CONFIG_SPARSEMEM_VMEMMAP */ > >> > >> -/** > >> - * clear_subsection_map - Clear subsection map of one memory region > >> - * > >> - * @pfn - start pfn of the memory range > >> - * @nr_pages - number of pfns to add in the region > >> - * > >> - * This is only intended for hotplug, and clear the related subsection > >> - * map inside one section. > >> - * > >> - * Return: > >> - * * -EINVAL - Section already deactived. > >> - * * 0 - Subsection map is emptied. > >> - * * 1 - Subsection map is not empty. > >> - */ > >> static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > >> { > >> DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; > >> @@ -758,11 +744,12 @@ static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > >> return -EINVAL; > >> > >> bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); > >> + return 0; > >> +} > >> > >> - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) > >> - return 0; > >> - > >> - return 1; > >> +static bool is_subsection_map_empty(unsigned long pfn, unsigned long nr_pages) > >> +{ > >> + return bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); > >> } > >> > >> static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > >> @@ -771,11 +758,8 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > >> struct mem_section *ms = __pfn_to_section(pfn); > >> bool section_is_early = early_section(ms); > >> struct page *memmap = NULL; > >> - int rc; > >> - > >> > >> - rc = clear_subsection_map(pfn, nr_pages); > >> - if (IS_ERR_VALUE((unsigned long)rc)) > >> + if (unlikely(clear_subsection_map(pfn, nr_pages))) > >> return; > >> /* > >> * There are 3 cases to handle across two configurations > >> @@ -794,7 +778,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > >> * > >> * For 2/ and 3/ the SPARSEMEM_VMEMMAP={y,n} cases are unified > >> */ > >> - if (!rc) { > >> + if (is_subsection_map_empty(pfn, nr_pages)) { > >> unsigned long section_nr = pfn_to_section_nr(pfn); > > > > Tried this way, it's not good in this patch. Since ms->usage might be > > freed in this place. > > > > if (!PageReserved(virt_to_page(ms->usage))) { > > kfree(ms->usage); > > ms->usage = NULL; > > } > > So your patch #1 is already broken. Just cache the result in patch #1. > > bool empty; Right, good catch. Will take this way. Thanks. > > ... > empty = bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION); > ... > if (empty) { > ... > }
diff --git a/mm/sparse.c b/mm/sparse.c index 977b47acd38d..df857ee9330c 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -726,14 +726,25 @@ 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) +/** + * clear_subsection_map - Clear subsection map of one memory region + * + * @pfn - start pfn of the memory range + * @nr_pages - number of pfns to add in the region + * + * This is only intended for hotplug, and clear the related subsection + * map inside one section. + * + * Return: + * * -EINVAL - Section already deactived. + * * 0 - Subsection map is emptied. + * * 1 - Subsection map is not empty. + */ +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; unsigned long *subsection_map = ms->usage ? &ms->usage->subsection_map[0] : NULL; @@ -744,8 +755,28 @@ 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; + return -EINVAL; + + bitmap_xor(subsection_map, map, subsection_map, SUBSECTIONS_PER_SECTION); + if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) + return 0; + + return 1; +} + +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; + int rc; + + + rc = clear_subsection_map(pfn, nr_pages); + if (IS_ERR_VALUE((unsigned long)rc)) + return; /* * There are 3 cases to handle across two configurations * (SPARSEMEM_VMEMMAP={y,n}): @@ -763,8 +794,7 @@ 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); - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) { + if (!rc) { unsigned long section_nr = pfn_to_section_nr(pfn); /* @@ -786,7 +816,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, else depopulate_section_memmap(pfn, nr_pages, altmap); - if (bitmap_empty(subsection_map, SUBSECTIONS_PER_SECTION)) + if (!rc) ms->section_mem_map = (unsigned long)NULL; }
Wrap the codes 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 | 46 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 8 deletions(-)