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
Signed-off-by: Cody P Schafer <[email protected]>
---
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
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 <[email protected]>
---
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
Signed-off-by: Cody P Schafer <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
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().
On Wed, 1 May 2013, Cody P Schafer wrote:
> Signed-off-by: Cody P Schafer <[email protected]>
Acked-by: David Rientjes <[email protected]>
On Wed, 1 May 2013, Cody P Schafer wrote:
> Signed-off-by: Cody P Schafer <[email protected]>
Nack, pgdat_resize_unlock() is unnecessary if irqs are known to be
disabled.
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.
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 <[email protected]>
>
> 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.
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 <[email protected]>
>
> 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?
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.
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.
On Wed, 1 May 2013, Cody P Schafer wrote:
> > > Signed-off-by: Cody P Schafer <[email protected]>
> >
> > 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.
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.
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.
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 <[email protected]>
>>>
>>> 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.
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().