[PATCH v4 1/3] mm/sparse: optimize sparse_index_alloc

[PATCH v4 1/3] mm/sparse: optimize sparse_index_alloc

[Gavin Shan]

With CONFIG_SPARSEMEM_EXTREME, the two level of memory section
descriptors are allocated from slab or bootmem. When allocating
from slab, let slab/bootmem allocator to clear the memory chunk.
We needn't clear that explicitly.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Reviewed-by: Michal Hocko <mhocko@suse.cz>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/sparse.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 6a4bf91..781fa04 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -65,14 +65,12 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
 
 	if (slab_is_available()) {
 		if (node_state(nid, N_HIGH_MEMORY))
-			section = kmalloc_node(array_size, GFP_KERNEL, nid);
+			section = kzalloc_node(array_size, GFP_KERNEL, nid);
 		else
-			section = kmalloc(array_size, GFP_KERNEL);
-	} else
+			section = kzalloc(array_size, GFP_KERNEL);
+	} else {
 		section = alloc_bootmem_node(NODE_DATA(nid), array_size);
-
-	if (section)
-		memset(section, 0, array_size);
+	}
 
 	return section;
 }
-- 
1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 2/3] mm/sparse: more check on mem_section number

[Gavin Shan]

Function __section_nr() was implemented to retrieve the corresponding
memory section number according to its descriptor. It's possible that
the specified memory section descriptor isn't existing in the global
array. So here to add more check on that and report error for wrong
case.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Acked-by: David Rientjes <rientjes@google.com>
---
 mm/sparse.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/sparse.c b/mm/sparse.c
index 781fa04..8b8edfb 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -130,6 +130,8 @@ int __section_nr(struct mem_section* ms)
 		     break;
 	}
 
+	VM_BUG_ON(root_nr == NR_SECTION_ROOTS);
+
 	return (root_nr * SECTIONS_PER_ROOT) + (ms - root);
 }
 
-- 
1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/3] mm/sparse: remove index_init_lock

[Gavin Shan]

Apart from call to sparse_index_init() during boot stage, the function
is mainly used for hotplug case as follows and protected by hotplug
mutex "mem_hotplug_mutex". So we needn't the spinlock in sparse_index_init().

	sparse_index_init
	sparse_add_one_section
	__add_section
	__add_pages
	arch_add_memory
	add_memory

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 mm/sparse.c |   14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 8b8edfb..4437c6c 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -77,7 +77,6 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
 
 static int __meminit sparse_index_init(unsigned long section_nr, int nid)
 {
-	static DEFINE_SPINLOCK(index_init_lock);
 	unsigned long root = SECTION_NR_TO_ROOT(section_nr);
 	struct mem_section *section;
 	int ret = 0;
@@ -88,20 +87,9 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
 	section = sparse_index_alloc(nid);
 	if (!section)
 		return -ENOMEM;
-	/*
-	 * This lock keeps two different sections from
-	 * reallocating for the same index
-	 */
-	spin_lock(&index_init_lock);
-
-	if (mem_section[root]) {
-		ret = -EEXIST;
-		goto out;
-	}
 
 	mem_section[root] = section;
-out:
-	spin_unlock(&index_init_lock);
+
 	return ret;
 }
 #else /* !SPARSEMEM_EXTREME */
-- 
1.7.9.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/3] mm/sparse: remove index_init_lock

[Michal Hocko]

On Fri 06-07-12 11:09:38, Gavin Shan wrote:
> Apart from call to sparse_index_init() during boot stage, the function
> is mainly used for hotplug case as follows and protected by hotplug

mainly? Who are the others?

> mutex "mem_hotplug_mutex". So we needn't the spinlock in sparse_index_init().

I think you are right but the changelog should be more convincing. It
would be also good to mention the origin motivation for the lock (I
couldn't find it in the history - Dave?).

> 
> 	sparse_index_init
> 	sparse_add_one_section
> 	__add_section
> 	__add_pages
> 	arch_add_memory
> 	add_memory
> 
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  mm/sparse.c |   14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 8b8edfb..4437c6c 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -77,7 +77,6 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
>  
>  static int __meminit sparse_index_init(unsigned long section_nr, int nid)
>  {
> -	static DEFINE_SPINLOCK(index_init_lock);
>  	unsigned long root = SECTION_NR_TO_ROOT(section_nr);
>  	struct mem_section *section;
>  	int ret = 0;
> @@ -88,20 +87,9 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
>  	section = sparse_index_alloc(nid);
>  	if (!section)
>  		return -ENOMEM;
> -	/*
> -	 * This lock keeps two different sections from
> -	 * reallocating for the same index
> -	 */
> -	spin_lock(&index_init_lock);
> -
> -	if (mem_section[root]) {
> -		ret = -EEXIST;
> -		goto out;
> -	}
>  
>  	mem_section[root] = section;
> -out:
> -	spin_unlock(&index_init_lock);
> +
>  	return ret;
>  }
>  #else /* !SPARSEMEM_EXTREME */
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/3] mm/sparse: remove index_init_lock

[Gavin Shan]

On Mon, Jul 09, 2012 at 01:13:04PM +0200, Michal Hocko wrote:
>On Fri 06-07-12 11:09:38, Gavin Shan wrote:
>> Apart from call to sparse_index_init() during boot stage, the function
>> is mainly used for hotplug case as follows and protected by hotplug
>
>mainly? Who are the others?
>

The function can possibilly be called during hotplug as well as boot stage
as follows. The others means "boot stage" here :-)

sparse_index_init
memory_present
sparse_memory_present_with_active_regions


>> mutex "mem_hotplug_mutex". So we needn't the spinlock in sparse_index_init().
>
>I think you are right but the changelog should be more convincing. It
>would be also good to mention the origin motivation for the lock (I
>couldn't find it in the history - Dave?).
>

Copy Dave's changelog for the original patch again.

---
sparse_index_init() is designed to be safe if two copies of it race.  It
uses "index_init_lock" to ensure that, even in the case of a race, only
one CPU will manage to do:

mem_section[root] = section;

However, in the case where two copies of sparse_index_init() _do_ race,
which is probablly caused by making online for multiple memory sections
that depend on same entry of array mem_section[] simultaneously from
different CPUs. 
---

Michal, How about the following changelog?

---

sparse_index_init() is designed to be safe if two copies of it race.  It
uses "index_init_lock" to ensure that, even in the case of a race, only
one CPU will manage to do:

mem_section[root] = section;

On the other hand, sparse_index_init() is possiblly called during system
boot stage and hotplug path as follows. We need't lock during system boot
stage to protect "mem_section[root]" and the function has been protected by
hotplug mutex "mem_hotplug_mutex" as well in hotplug case. So we needn't the
spinklock in the function.

---


Thanks,
Gavin

>> 
>> 	sparse_index_init
>> 	sparse_add_one_section
>> 	__add_section
>> 	__add_pages
>> 	arch_add_memory
>> 	add_memory
>> 
>> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>> ---
>>  mm/sparse.c |   14 +-------------
>>  1 file changed, 1 insertion(+), 13 deletions(-)
>> 
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index 8b8edfb..4437c6c 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -77,7 +77,6 @@ static struct mem_section noinline __init_refok *sparse_index_alloc(int nid)
>>  
>>  static int __meminit sparse_index_init(unsigned long section_nr, int nid)
>>  {
>> -	static DEFINE_SPINLOCK(index_init_lock);
>>  	unsigned long root = SECTION_NR_TO_ROOT(section_nr);
>>  	struct mem_section *section;
>>  	int ret = 0;
>> @@ -88,20 +87,9 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
>>  	section = sparse_index_alloc(nid);
>>  	if (!section)
>>  		return -ENOMEM;
>> -	/*
>> -	 * This lock keeps two different sections from
>> -	 * reallocating for the same index
>> -	 */
>> -	spin_lock(&index_init_lock);
>> -
>> -	if (mem_section[root]) {
>> -		ret = -EEXIST;
>> -		goto out;
>> -	}
>>  
>>  	mem_section[root] = section;
>> -out:
>> -	spin_unlock(&index_init_lock);
>> +
>>  	return ret;
>>  }
>>  #else /* !SPARSEMEM_EXTREME */
>> -- 
>> 1.7.9.5
>> 
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
>-- 
>Michal Hocko
>SUSE Labs
>SUSE LINUX s.r.o.
>Lihovarska 1060/12
>190 00 Praha 9    
>Czech Republic
>
>--
>To unsubscribe, send a message with 'unsubscribe linux-mm' in
>the body to majordomo@kvack.org.  For more info on Linux MM,
>see: http://www.linux-mm.org/ .
>Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/3] mm/sparse: remove index_init_lock

[Michal Hocko]

On Mon 09-07-12 19:59:35, Gavin Shan wrote:
[...]
> Michal, How about the following changelog?
> 
> ---
> 
> sparse_index_init() is designed to be safe if two copies of it race.  It
> uses "index_init_lock" to ensure that, even in the case of a race, only
> one CPU will manage to do:
> 
> mem_section[root] = section;
> 
> On the other hand, sparse_index_init() is possiblly called during system
> boot stage and hotplug path as follows. We need't lock during system boot
> stage to protect "mem_section[root]" and the function has been protected by
> hotplug mutex "mem_hotplug_mutex" as well in hotplug case. So we needn't the
> spinklock in the function.

The changelog is still hard to read but it's getting there slowly ;)
What about the following?
---
sparse_index_init uses index_init_lock spinlock to protect root
mem_section assignment. The lock is not necessary anymore because the
function is called only during the boot (during paging init which
is executed only from a single CPU) and from the hotplug code (by
add_memory via arch_add_memory) which uses mem_hotplug_mutex.

The lock has been introduced by 28ae55c9 (sparsemem extreme: hotplug
preparation) and sparse_index_init was used only during boot at that
time. 
Later when the hotplug code (and add_memory) was introduced there was
no synchronization so it was possible to online more sections from
the same root probably (though I am not 100% sure about that).
The first synchronization has been added by 6ad696d2 (mm: allow memory
hotplug and hibernation in the same kernel) which has been later
replaced by the mem_hotplug_mutex - 20d6c96b (mem-hotplug: introduce
{un}lock_memory_hotplug()).

Let's remove the lock as it is not needed and it makes the code more
confusing.
---

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/3] mm/sparse: remove index_init_lock

[Gavin Shan]

On Tue, Jul 10, 2012 at 05:59:41PM +0200, Michal Hocko wrote:
>On Mon 09-07-12 19:59:35, Gavin Shan wrote:
>[...]
>> Michal, How about the following changelog?
>> 
>> ---
>> 
>> sparse_index_init() is designed to be safe if two copies of it race.  It
>> uses "index_init_lock" to ensure that, even in the case of a race, only
>> one CPU will manage to do:
>> 
>> mem_section[root] = section;
>> 
>> On the other hand, sparse_index_init() is possiblly called during system
>> boot stage and hotplug path as follows. We need't lock during system boot
>> stage to protect "mem_section[root]" and the function has been protected by
>> hotplug mutex "mem_hotplug_mutex" as well in hotplug case. So we needn't the
>> spinklock in the function.
>
>The changelog is still hard to read but it's getting there slowly ;)
>What about the following?

Thanks, Michal. I will resend the patch with your changelog :-)

Thanks for your time.

Gavin

>---
>sparse_index_init uses index_init_lock spinlock to protect root
>mem_section assignment. The lock is not necessary anymore because the
>function is called only during the boot (during paging init which
>is executed only from a single CPU) and from the hotplug code (by
>add_memory via arch_add_memory) which uses mem_hotplug_mutex.
>
>The lock has been introduced by 28ae55c9 (sparsemem extreme: hotplug
>preparation) and sparse_index_init was used only during boot at that
>time. 
>Later when the hotplug code (and add_memory) was introduced there was
>no synchronization so it was possible to online more sections from
>the same root probably (though I am not 100% sure about that).
>The first synchronization has been added by 6ad696d2 (mm: allow memory
>hotplug and hibernation in the same kernel) which has been later
>replaced by the mem_hotplug_mutex - 20d6c96b (mem-hotplug: introduce
>{un}lock_memory_hotplug()).
>
>Let's remove the lock as it is not needed and it makes the code more
>confusing.
>---
>
>-- 
>Michal Hocko
>SUSE Labs
>SUSE LINUX s.r.o.
>Lihovarska 1060/12
>190 00 Praha 9    
>Czech Republic
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>