[PATCH] mm/page_alloc: don't re-init pageset in zone_pcp_update()

[PATCH] mm/page_alloc: don't re-init pageset in zone_pcp_update()

[Cody P Schafer]

Factor pageset_set_high_and_batch() (which contains all needed logic too
set a pageset's ->high and ->batch inrespective of system state) out of
zone_pageset_init(), which avoids us calling pageset_init(), and
unsafely blowing away a pageset at runtime (leaked pages and
potentially some funky allocations would be the result) when memory
hotplug is triggered.

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

Unless memory hotplug is being triggered on boot, this should *not* be cause of Valdis
Kletnieks' reported bug in -next:
         "next-20130607 BUG: Bad page state in process systemd pfn:127643"

---

 mm/page_alloc.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 18102e1..f62c7ac 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4111,11 +4111,9 @@ static void pageset_set_high(struct per_cpu_pageset *p,
 	pageset_update(&p->pcp, high, batch);
 }
 
-static void __meminit zone_pageset_init(struct zone *zone, int cpu)
+static void __meminit pageset_set_high_and_batch(struct zone *zone,
+		struct per_cpu_pageset *pcp)
 {
-	struct per_cpu_pageset *pcp = per_cpu_ptr(zone->pageset, cpu);
-
-	pageset_init(pcp);
 	if (percpu_pagelist_fraction)
 		pageset_set_high(pcp,
 			(zone->managed_pages /
@@ -4124,6 +4122,14 @@ static void __meminit zone_pageset_init(struct zone *zone, int cpu)
 		pageset_set_batch(pcp, zone_batchsize(zone));
 }
 
+static void __meminit zone_pageset_init(struct zone *zone, int cpu)
+{
+	struct per_cpu_pageset *pcp = per_cpu_ptr(zone->pageset, cpu);
+
+	pageset_init(pcp);
+	pageset_set_high_and_batch(zone, pcp);
+}
+
 static void __meminit setup_zone_pageset(struct zone *zone)
 {
 	int cpu;
@@ -6173,7 +6179,8 @@ void __meminit zone_pcp_update(struct zone *zone)
 	unsigned cpu;
 	mutex_lock(&pcp_batch_high_lock);
 	for_each_possible_cpu(cpu)
-		zone_pageset_init(zone, cpu);
+		pageset_set_high_and_batch(zone,
+				per_cpu_ptr(zone->pageset, cpu));
 	mutex_unlock(&pcp_batch_high_lock);
 }
 #endif
-- 
1.8.3

--
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] mm/page_alloc: don't re-init pageset in zone_pcp_update()

[Andrew Morton]

On Tue, 11 Jun 2013 15:12:59 -0700 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:

> Factor pageset_set_high_and_batch() (which contains all needed logic too
> set a pageset's ->high and ->batch inrespective of system state) out of
> zone_pageset_init(), which avoids us calling pageset_init(), and
> unsafely blowing away a pageset at runtime (leaked pages and
> potentially some funky allocations would be the result) when memory
> hotplug is triggered.

This changelog is pretty screwed up :( It tells us what the patch does
but not why it does it.

> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
> ---
> 
> Unless memory hotplug is being triggered on boot, this should *not* be cause of Valdis
> Kletnieks' reported bug in -next:
>          "next-20130607 BUG: Bad page state in process systemd pfn:127643"

And this addendum appears to hint at the info we need.

Please, send a new changelog?  That should include a description of the
user-visible effects of the bug which is being fixed, a description of
why it occurs and a description of how it was fixed.  It would also be
helpful if you can identify which kernel version(s) need the fix.

Also, a Reported-by:Valdis would be appropriate.

Thanks.

--
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] mm/page_alloc: don't re-init pageset in zone_pcp_update()

[Cody P Schafer]

On 06/12/2013 02:20 PM, Andrew Morton wrote:
> On Tue, 11 Jun 2013 15:12:59 -0700 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:
>
>> Factor pageset_set_high_and_batch() (which contains all needed logic too
>> set a pageset's ->high and ->batch inrespective of system state) out of
>> zone_pageset_init(), which avoids us calling pageset_init(), and
>> unsafely blowing away a pageset at runtime (leaked pages and
>> potentially some funky allocations would be the result) when memory
>> hotplug is triggered.
>
> This changelog is pretty screwed up :( It tells us what the patch does
> but not why it does it.
>

It says why it does it, though perhaps a bit hidden:
 >  avoids us calling pageset_init(), and unsafely blowing away a pageset

>> Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com>
>> ---
>>
>> Unless memory hotplug is being triggered on boot, this should *not* be cause of Valdis
>> Kletnieks' reported bug in -next:
>>           "next-20130607 BUG: Bad page state in process systemd pfn:127643"
>
> And this addendum appears to hint at the info we need.

Note the *not*. I included this note only because I expected there would 
be a question of whether Valdis's reported bug was caused by this. It 
_isn't_. The bug this fix fixes is only triggered by memory_hotplug, and 
Valdis's bug occurred on boot.

> Please, send a new changelog?  That should include a description of the
> user-visible effects of the bug which is being fixed,  a description of
> why it occurs and a description of how it was fixed.It would also be
> helpful if you can identify which kernel version(s) need the fix.

It's just a -mm issue. It was introduced by my patchset starting with 
"mm/page_alloc: factor out setting of pcp->high and pcp->batch", where 
the actual place the bug snuck in was "mm/page_alloc: in 
zone_pcp_update(), uze zone_pageset_init()".

>
> Also, a Reported-by:Valdis would be appropriate.
>
I'm fine with adding it (I did take a look at my page_alloc.c changes 
because he reported that bug), but as mentioned before, this fixes a 
different bug.

Anyhow, a reorganized (and clearer) changelog with the same content follows:
---
mm/page_alloc: don't re-init pageset in zone_pcp_update()

When memory hotplug is triggered, we call pageset_init() on a 
per-cpu-pageset which both contains pages and is in use, causing both 
the leakage of those pages and (potentially) bad behaviour if a page is 
allocated from the pageset while it is being cleared.

Avoid this by factoring pageset_set_high_and_batch() (which contains all 
needed logic too set a pageset's ->high and ->batch inrespective of 
system state), and using that instead of zone_pageset_init() in 
zone_pcp_update().

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.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>

Re: [PATCH] mm/page_alloc: don't re-init pageset in zone_pcp_update()

[Cody P Schafer]

> Anyhow, a reorganized (and clearer) changelog with the same content
> follows:
> ---

I made a few wording tweaks:
---
mm/page_alloc: don't re-init pageset in zone_pcp_update()

When memory hotplug is triggered, we call pageset_init() on
per-cpu-pagesets which both contain pages and are in use, causing both
the leakage of those pages and (potentially) bad behaviour if a page is
allocated from a pageset while it is being cleared.

Avoid this by factoring out pageset_set_high_and_batch() (which contains 
all needed logic too set a pageset's ->high and ->batch inrespective of 
system state) from zone_pageset_init() and using the new 
pageset_set_high_and_batch() instead of zone_pageset_init() in 
zone_pcp_update().

Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.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>