Re: [PATCH v2 3/7] mm/sparse.c: introduce a new function clear_subsection_map()

[David Hildenbrand]

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