[PATCH 0/4] misc patches related to resizing nodes & zones

[PATCH 0/4] misc patches related to resizing nodes & zones

[Cody P Schafer]

The first 3 are simply comment fixes and clarifications on locking.

The last 1 adds additional locking when updating node_present_pages based on
the existing documentation.

Cody P Schafer (4):
  mmzone: make holding lock_memory_hotplug() a requirement for updating
    pgdat size
  mm: fix comment referring to non-existent size_seqlock, change to
    span_seqlock
  mmzone: note that node_size_lock should be manipulated via
    pgdat_resize_lock()
  memory_hotplug: use pgdat_resize_lock() when updating
    node_present_pages

 include/linux/mmzone.h | 7 ++++++-
 mm/memory_hotplug.c    | 5 +++++
 2 files changed, 11 insertions(+), 1 deletion(-)

-- 
1.8.2.2

--
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 1/4] mmzone: make holding lock_memory_hotplug() a requirement for updating pgdat size

[Cody P Schafer]

All updaters of pgdat size (spanned_pages, start_pfn, and
present_pages) currently also hold lock_memory_hotplug() (in addition
to pgdat_resize_lock()).

Document this and make holding of that lock a requirement on the update
side for now, but keep the pgdat_resize_lock() around for readers that
can't lock a mutex.

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
 include/linux/mmzone.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 5c76737..09ac172 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -716,6 +716,9 @@ typedef struct pglist_data {
 	 * or node_spanned_pages stay constant.  Holding this will also
 	 * guarantee that any pfn_valid() stays that way.
 	 *
+	 * Updaters of any of these fields also must hold
+	 * lock_memory_hotplug().
+	 *
 	 * Nests above zone->lock and zone->size_seqlock.
 	 */
 	spinlock_t node_size_lock;
-- 
1.8.2.2

--
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 1/4] mmzone: make holding lock_memory_hotplug() a requirement for updating pgdat size

[David Rientjes]

On Wed, 1 May 2013, Cody P Schafer wrote:

> All updaters of pgdat size (spanned_pages, start_pfn, and
> present_pages) currently also hold lock_memory_hotplug() (in addition
> to pgdat_resize_lock()).
> 
> Document this and make holding of that lock a requirement on the update
> side for now, but keep the pgdat_resize_lock() around for readers that
> can't lock a mutex.
> 
> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>

Nack, these fields are initialized at boot without lock_memory_hotplug(), 
so you're statement is wrong, and all you need is pgdat_resize_lock().

--
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 1/4] mmzone: make holding lock_memory_hotplug() a requirement for updating pgdat size

[Cody P Schafer]

On 05/01/2013 03:26 PM, David Rientjes wrote:
> On Wed, 1 May 2013, Cody P Schafer wrote:
>
>> All updaters of pgdat size (spanned_pages, start_pfn, and
>> present_pages) currently also hold lock_memory_hotplug() (in addition
>> to pgdat_resize_lock()).
>>
>> Document this and make holding of that lock a requirement on the update
>> side for now, but keep the pgdat_resize_lock() around for readers that
>> can't lock a mutex.
>>
>> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
>
> Nack, these fields are initialized at boot without lock_memory_hotplug(),
> so you're statement is wrong, and all you need is pgdat_resize_lock().

They are also initialized at boot without pgdat_resize_lock(), if we 
consider boot time, quite a few of the statements on when locking is 
required are wrong.

That said, you are correct that it is not strictly required to hold 
lock_memory_hotplug() when updating the fields in question because 
pgdat_resize_lock() is used.



--
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 1/4] mmzone: make holding lock_memory_hotplug() a requirement for updating pgdat size

[David Rientjes]

On Wed, 1 May 2013, Cody P Schafer wrote:

> They are also initialized at boot without pgdat_resize_lock(), if we consider
> boot time, quite a few of the statements on when locking is required are
> wrong.
> 
> That said, you are correct that it is not strictly required to hold
> lock_memory_hotplug() when updating the fields in question because
> pgdat_resize_lock() is used.
> 

I think you've confused node size fields with zone size fields.

--
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 1/4] mmzone: make holding lock_memory_hotplug() a requirement for updating pgdat size

[Cody P Schafer]

On 05/01/2013 03:39 PM, David Rientjes wrote:
> On Wed, 1 May 2013, Cody P Schafer wrote:
>
>> They are also initialized at boot without pgdat_resize_lock(), if we consider
>> boot time, quite a few of the statements on when locking is required are
>> wrong.
>>
>> That said, you are correct that it is not strictly required to hold
>> lock_memory_hotplug() when updating the fields in question because
>> pgdat_resize_lock() is used.
>>
>
> I think you've confused node size fields with zone size fields.
>

Where? I'm afraid I don't see where I'm mixing them.


--
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 2/4] mm: fix comment referring to non-existent size_seqlock, change to span_seqlock

[Cody P Schafer]

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
 include/linux/mmzone.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 09ac172..afd0aa5 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -719,7 +719,7 @@ typedef struct pglist_data {
 	 * Updaters of any of these fields also must hold
 	 * lock_memory_hotplug().
 	 *
-	 * Nests above zone->lock and zone->size_seqlock.
+	 * Nests above zone->lock and zone->span_seqlock
 	 */
 	spinlock_t node_size_lock;
 #endif
-- 
1.8.2.2

--
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 2/4] mm: fix comment referring to non-existent size_seqlock, change to span_seqlock

[David Rientjes]

On Wed, 1 May 2013, Cody P Schafer wrote:

> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>

Acked-by: David Rientjes <rientjes@google.com>

--
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/4] mmzone: note that node_size_lock should be manipulated via pgdat_resize_lock()

[Cody P Schafer]

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
 include/linux/mmzone.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index afd0aa5..45be383 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -716,6 +716,8 @@ typedef struct pglist_data {
 	 * or node_spanned_pages stay constant.  Holding this will also
 	 * guarantee that any pfn_valid() stays that way.
 	 *
+	 * Use pgdat_resize_lock() and pgdat_resize_unlock() to manipulate.
+	 *
 	 * Updaters of any of these fields also must hold
 	 * lock_memory_hotplug().
 	 *
-- 
1.8.2.2

--
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/4] mmzone: note that node_size_lock should be manipulated via pgdat_resize_lock()

[David Rientjes]

On Wed, 1 May 2013, Cody P Schafer wrote:

> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>

Nack, pgdat_resize_unlock() is unnecessary if irqs are known to be 
disabled.

--
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/4] mmzone: note that node_size_lock should be manipulated via pgdat_resize_lock()

[Cody P Schafer]

On 05/01/2013 03:29 PM, David Rientjes wrote:
> On Wed, 1 May 2013, Cody P Schafer wrote:
>
>> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
>
> Nack, pgdat_resize_unlock() is unnecessary if irqs are known to be
> disabled.
>

All this patch does is is indicate that rather than using node_size_lock 
directly (as it won't be around without CONFIG_MEMORY_HOTPLUG), one 
should use the pgdat_resize_[un]lock() helper macros.

And yes, _strictly_ speaking, one could want to avoid the 
spin_lock_irqsave/restore that pgdat_resize_*lock() does.

Right now we don't provide helpers that do that. Do you see a need for them?

--
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/4] mmzone: note that node_size_lock should be manipulated via pgdat_resize_lock()

[David Rientjes]

On Wed, 1 May 2013, Cody P Schafer wrote:

> > > Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
> > 
> > Nack, pgdat_resize_unlock() is unnecessary if irqs are known to be
> > disabled.
> > 
> 
> All this patch does is is indicate that rather than using node_size_lock
> directly (as it won't be around without CONFIG_MEMORY_HOTPLUG), one should use
> the pgdat_resize_[un]lock() helper macros.
> 

I think that's obvious given the lock is surrounded by
#ifdef CONFIG_MEMORY_HOTPLUG.  The fact remains that hotplug code need not 
use pgdat_resize_lock() if irqs are disabled.

--
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/4] mmzone: note that node_size_lock should be manipulated via pgdat_resize_lock()

[Cody P Schafer]

On 05/01/2013 03:42 PM, David Rientjes wrote:
> On Wed, 1 May 2013, Cody P Schafer wrote:
>
>>>> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
>>>
>>> Nack, pgdat_resize_unlock() is unnecessary if irqs are known to be
>>> disabled.
>>>
>>
>> All this patch does is is indicate that rather than using node_size_lock
>> directly (as it won't be around without CONFIG_MEMORY_HOTPLUG), one should use
>> the pgdat_resize_[un]lock() helper macros.
>>
>
> I think that's obvious given the lock is surrounded by
> #ifdef CONFIG_MEMORY_HOTPLUG.  The fact remains that hotplug code need not
> use pgdat_resize_lock() if irqs are disabled.
>

Obvious how? This comment is the documentation on how to handle locking 
of pg_data_t, and doesn't mention pgdat_resize_lock() at all. Sure, a 
newcomer would probably find pgdat_resize_lock() eventually, even more 
so if they were interested in performance gains from not re-disabling 
local irqs.

I don't see a convincing reason to omit relevant documentation and make 
it more difficult to find the "right" way to do things.

--
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 4/4] memory_hotplug: use pgdat_resize_lock() when updating node_present_pages

[Cody P Schafer]

mmzone.h documents node_size_lock (which pgdat_resize_lock() locks) as
guarding against changes to node_present_pages, so actually lock it when
we update node_present_pages to keep that promise.

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
---
 mm/memory_hotplug.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a221fac..0bdca10 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -915,6 +915,7 @@ static void node_states_set_node(int node, struct memory_notify *arg)
 
 int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_type)
 {
+	unsigned long flags;
 	unsigned long onlined_pages = 0;
 	struct zone *zone;
 	int need_zonelists_rebuild = 0;
@@ -993,7 +994,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
 
 	zone->managed_pages += onlined_pages;
 	zone->present_pages += onlined_pages;
+
+	pgdat_resize_lock(zone->zone_pgdat, &flags);
 	zone->zone_pgdat->node_present_pages += onlined_pages;
+	pgdat_resize_unlock(zone->zone_pgdat, &flags);
+
 	if (onlined_pages) {
 		node_states_set_node(zone_to_nid(zone), &arg);
 		if (need_zonelists_rebuild)
-- 
1.8.2.2

--
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 4/4] memory_hotplug: use pgdat_resize_lock() when updating node_present_pages

[David Rientjes]

On Wed, 1 May 2013, Cody P Schafer wrote:

> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index a221fac..0bdca10 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -915,6 +915,7 @@ static void node_states_set_node(int node, struct memory_notify *arg)
>  
>  int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_type)
>  {
> +	unsigned long flags;
>  	unsigned long onlined_pages = 0;
>  	struct zone *zone;
>  	int need_zonelists_rebuild = 0;
> @@ -993,7 +994,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>  
>  	zone->managed_pages += onlined_pages;
>  	zone->present_pages += onlined_pages;
> +
> +	pgdat_resize_lock(zone->zone_pgdat, &flags);
>  	zone->zone_pgdat->node_present_pages += onlined_pages;
> +	pgdat_resize_unlock(zone->zone_pgdat, &flags);
> +
>  	if (onlined_pages) {
>  		node_states_set_node(zone_to_nid(zone), &arg);
>  		if (need_zonelists_rebuild)

Why?  You can't get a partial read of a word-sized data structure.

--
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 4/4] memory_hotplug: use pgdat_resize_lock() when updating node_present_pages

[Cody P Schafer]

On 05/01/2013 03:30 PM, David Rientjes wrote:
> On Wed, 1 May 2013, Cody P Schafer wrote:
>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index a221fac..0bdca10 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -915,6 +915,7 @@ static void node_states_set_node(int node, struct memory_notify *arg)
>>
>>   int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_type)
>>   {
>> +	unsigned long flags;
>>   	unsigned long onlined_pages = 0;
>>   	struct zone *zone;
>>   	int need_zonelists_rebuild = 0;
>> @@ -993,7 +994,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>>
>>   	zone->managed_pages += onlined_pages;
>>   	zone->present_pages += onlined_pages;
>> +
>> +	pgdat_resize_lock(zone->zone_pgdat, &flags);
>>   	zone->zone_pgdat->node_present_pages += onlined_pages;
>> +	pgdat_resize_unlock(zone->zone_pgdat, &flags);
>> +
>>   	if (onlined_pages) {
>>   		node_states_set_node(zone_to_nid(zone), &arg);
>>   		if (need_zonelists_rebuild)
>
> Why?  You can't get a partial read of a word-sized data structure.
>

Guaranteed to be stable means that if I'm a reader and 
pgdat_resize_lock(), node_present_pages had better not change at all 
until I pgdat_resize_unlock().

If nothing needs this guarantee, we should change the rules of 
pgdat_resize_lock(). I played it safe and went with following the 
existing rules.

--
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 4/4] memory_hotplug: use pgdat_resize_lock() when updating node_present_pages

[David Rientjes]

On Wed, 1 May 2013, Cody P Schafer wrote:

> Guaranteed to be stable means that if I'm a reader and pgdat_resize_lock(),
> node_present_pages had better not change at all until I pgdat_resize_unlock().
> 
> If nothing needs this guarantee, we should change the rules of
> pgdat_resize_lock(). I played it safe and went with following the existing
> rules.
> 

__offline_pages() breaks your guarantee.

--
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 4/4] memory_hotplug: use pgdat_resize_lock() when updating node_present_pages

[Cody P Schafer]

On 05/01/2013 03:48 PM, David Rientjes wrote:
> On Wed, 1 May 2013, Cody P Schafer wrote:
>
>> Guaranteed to be stable means that if I'm a reader and pgdat_resize_lock(),
>> node_present_pages had better not change at all until I pgdat_resize_unlock().
>>
>> If nothing needs this guarantee, we should change the rules of
>> pgdat_resize_lock(). I played it safe and went with following the existing
>> rules.
>>
>
> __offline_pages() breaks your guarantee.
>

Thanks for pointing that out. Seems I fixed online_pages() but missed 
__offline_pages().

--
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>