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@xxxxxxxxxx>
>>> ---
>>> 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;
}
--
Thanks,
David / dhildenb
