[PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[kosaki.motohiro]

From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
another miuse still exist.

This patch fixes it.

Cc: David Rientjes <rientjes@google.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Minchan Kim <minchan.kim@gmail.com>
Cc: Wu Fengguang <fengguang.wu@intel.com>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 mm/page_alloc.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5716b00..4e7059d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1157,8 +1157,10 @@ void drain_zone_pages(struct zone *zone, struct per_cpu_pages *pcp)
 		to_drain = pcp->batch;
 	else
 		to_drain = pcp->count;
-	free_pcppages_bulk(zone, to_drain, pcp);
-	pcp->count -= to_drain;
+	if (to_drain > 0) {
+		free_pcppages_bulk(zone, to_drain, pcp);
+		pcp->count -= to_drain;
+	}
 	local_irq_restore(flags);
 }
 #endif
@@ -3914,7 +3916,8 @@ static int __zone_pcp_update(void *data)
 		pcp = &pset->pcp;
 
 		local_irq_save(flags);
-		free_pcppages_bulk(zone, pcp->count, pcp);
+		if (pcp->count > 0)
+			free_pcppages_bulk(zone, pcp->count, pcp);
 		setup_pageset(pset, batch);
 		local_irq_restore(flags);
 	}
-- 
1.7.1

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[Rik van Riel]

On 06/14/2012 12:16 PM, kosaki.motohiro@gmail.com wrote:
> From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>
> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
> another miuse still exist.
>
> This patch fixes it.
>
> Cc: David Rientjes<rientjes@google.com>
> Cc: Mel Gorman<mel@csn.ul.ie>
> Cc: Johannes Weiner<hannes@cmpxchg.org>
> Cc: Minchan Kim<minchan.kim@gmail.com>
> Cc: Wu Fengguang<fengguang.wu@intel.com>
> Cc: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Rik van Riel<riel@redhat.com>
> Cc: Andrew Morton<akpm@linux-foundation.org>
> Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>

Acked-by: Rik van Riel <riel@redhat.com>

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[Minchan Kim]

On 06/15/2012 01:16 AM, kosaki.motohiro@gmail.com wrote:

> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
> another miuse still exist.
> 
> This patch fixes it.
> 
> Cc: David Rientjes <rientjes@google.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> Cc: Wu Fengguang <fengguang.wu@intel.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Reviewed-by: Minchan Kim <minchan@kernel.org>

Just nitpick.
Personally, I want to fix it follwing as 
It's more simple and reduce vulnerable error in future.

If you mind, go ahead with your version. I am not against with it, either.

barrios@bbox:~/linux-next$ git diff
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4403009..a32ac56 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -637,6 +637,12 @@ static void free_pcppages_bulk(struct zone *zone, int count,
        int batch_free = 0;
        int to_free = count;
 
+       /*
+        * Let's avoid unnecessary reset of pages_scanned.
+        */
+       if (!count)
+               return;
+
        spin_lock(&zone->lock);
        zone->all_unreclaimable = 0;
        zone->pages_scanned = 0;
@@ -1175,6 +1181,7 @@ static void drain_pages(unsigned int cpu)
 {
        unsigned long flags;
        struct zone *zone;
+       int to_drain;
 
        for_each_populated_zone(zone) {
                struct per_cpu_pageset *pset;
@@ -1184,10 +1191,9 @@ static void drain_pages(unsigned int cpu)
                pset = per_cpu_ptr(zone->pageset, cpu);
 
                pcp = &pset->pcp;
-               if (pcp->count) {
-                       free_pcppages_bulk(zone, pcp->count, pcp);
-                       pcp->count = 0;
-               }
+               to_drain = pcp->count;
+               free_pcppages_bulk(zone, to_drain, pcp);
+               pcp->count -= to_drain;
                local_irq_restore(flags);
        }
 }

-- 
Kind regards,
Minchan Kim

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[Mel Gorman]

On Thu, Jun 14, 2012 at 12:16:10PM -0400, kosaki.motohiro@gmail.com wrote:
> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
> another miuse still exist.
> 
> This patch fixes it.
> 
> Cc: David Rientjes <rientjes@google.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> Cc: Wu Fengguang <fengguang.wu@intel.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Acked-by: Mel Gorman <mel@csn.ul.ie>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[KOSAKI Motohiro]

(6/15/12 3:19 AM), Minchan Kim wrote:
> On 06/15/2012 01:16 AM, kosaki.motohiro@gmail.com wrote:
>
>> From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>>
>> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
>> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
>> another miuse still exist.
>>
>> This patch fixes it.
>>
>> Cc: David Rientjes<rientjes@google.com>
>> Cc: Mel Gorman<mel@csn.ul.ie>
>> Cc: Johannes Weiner<hannes@cmpxchg.org>
>> Cc: Minchan Kim<minchan.kim@gmail.com>
>> Cc: Wu Fengguang<fengguang.wu@intel.com>
>> Cc: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>> Cc: Rik van Riel<riel@redhat.com>
>> Cc: Andrew Morton<akpm@linux-foundation.org>
>> Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>
> Reviewed-by: Minchan Kim<minchan@kernel.org>
>
> Just nitpick.
> Personally, I want to fix it follwing as
> It's more simple and reduce vulnerable error in future.
>
> If you mind, go ahead with your version. I am not against with it, either.

I don't like your version because free_pcppages_bulk() can be called from
free_pages() hotpath. then, i wouldn't like to put a branch if we can avoid it.

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[Johannes Weiner]

On Thu, Jun 14, 2012 at 12:16:10PM -0400, kosaki.motohiro@gmail.com wrote:
> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
> another miuse still exist.
> 
> This patch fixes it.
> 
> Cc: David Rientjes <rientjes@google.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> Cc: Wu Fengguang <fengguang.wu@intel.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[Minchan Kim]

On Fri, Jun 15, 2012 at 11:52:34AM -0400, KOSAKI Motohiro wrote:
> (6/15/12 3:19 AM), Minchan Kim wrote:
> >On 06/15/2012 01:16 AM, kosaki.motohiro@gmail.com wrote:
> >
> >>From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> >>
> >>commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
> >>to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
> >>another miuse still exist.
> >>
> >>This patch fixes it.
> >>
> >>Cc: David Rientjes<rientjes@google.com>
> >>Cc: Mel Gorman<mel@csn.ul.ie>
> >>Cc: Johannes Weiner<hannes@cmpxchg.org>
> >>Cc: Minchan Kim<minchan.kim@gmail.com>
> >>Cc: Wu Fengguang<fengguang.wu@intel.com>
> >>Cc: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> >>Cc: Rik van Riel<riel@redhat.com>
> >>Cc: Andrew Morton<akpm@linux-foundation.org>
> >>Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> >
> >Reviewed-by: Minchan Kim<minchan@kernel.org>
> >
> >Just nitpick.
> >Personally, I want to fix it follwing as
> >It's more simple and reduce vulnerable error in future.
> >
> >If you mind, go ahead with your version. I am not against with it, either.
> 
> I don't like your version because free_pcppages_bulk() can be called from
> free_pages() hotpath. then, i wouldn't like to put a branch if we can avoid it.

Fair enough.

Thanks.

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[Kamezawa Hiroyuki]

(2012/06/15 1:16), kosaki.motohiro@gmail.com wrote:
> From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> 
> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
> another miuse still exist.
> 
> This patch fixes it.
> 
> Cc: David Rientjes<rientjes@google.com>
> Cc: Mel Gorman<mel@csn.ul.ie>
> Cc: Johannes Weiner<hannes@cmpxchg.org>
> Cc: Minchan Kim<minchan.kim@gmail.com>
> Cc: Wu Fengguang<fengguang.wu@intel.com>
> Cc: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Rik van Riel<riel@redhat.com>
> Cc: Andrew Morton<akpm@linux-foundation.org>
> Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[David Rientjes]

On Thu, 14 Jun 2012, kosaki.motohiro@gmail.com wrote:

> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
> another miuse still exist.
> 
> This patch fixes it.
> 
> Cc: David Rientjes <rientjes@google.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> Cc: Wu Fengguang <fengguang.wu@intel.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

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

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[Andrew Morton]

On Thu, 14 Jun 2012 12:16:10 -0400
kosaki.motohiro@gmail.com wrote:

> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
> another miuse still exist.

This changelog is irritating.  One can understand it a bit if one
happens to have a git repo handy (and why do this to the reader?), but
the changelog for 2ff754fa8f indicates that the patch might fix a
livelock.  Is that true of this patch?  Who knows...

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[KOSAKI Motohiro]

On Fri, Jun 22, 2012 at 4:19 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 14 Jun 2012 12:16:10 -0400
> kosaki.motohiro@gmail.com wrote:
>
>> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
>> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
>> another miuse still exist.
>
> This changelog is irritating.  One can understand it a bit if one
> happens to have a git repo handy (and why do this to the reader?), but
> the changelog for 2ff754fa8f indicates that the patch might fix a
> livelock.  Is that true of this patch?  Who knows...

The code in this simple patch speak the right usage, isn't it? And yes,
this patch also fixes a possibility of live lock. (but i haven't seen actual
live lock cause from this mistake)

When anyone find a function misuse and fixes it, He/She should confirm other
callsite and should all of mistake too. Otherwise we observe the same issue
sooner of later.

Re: [PATCH] mm: clear pages_scanned only if draining a pcp adds pages to the buddy allocator again

[Andrew Morton]

On Fri, 22 Jun 2012 17:10:59 -0400
KOSAKI Motohiro <kosaki.motohiro@gmail.com> wrote:

> On Fri, Jun 22, 2012 at 4:19 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Thu, 14 Jun 2012 12:16:10 -0400
> > kosaki.motohiro@gmail.com wrote:
> >
> >> commit 2ff754fa8f (mm: clear pages_scanned only if draining a pcp adds pages
> >> to the buddy allocator again) fixed one free_pcppages_bulk() misuse. But two
> >> another miuse still exist.
> >
> > This changelog is irritating. __One can understand it a bit if one
> > happens to have a git repo handy (and why do this to the reader?), but
> > the changelog for 2ff754fa8f indicates that the patch might fix a
> > livelock. __Is that true of this patch? __Who knows...
> 
> The code in this simple patch speak the right usage, isn't it?

It depends who is listening.

Please, put yourself in the position of poor-scmuck@linux-distro.com
who is reading your patch and wondering whether it will fix some
customer bug report he is working on.  Or wondering whether he should
backport it into his company's next kernel release.  He simply won't be
able to do this with the information which was provided here.  And if
we don't tell him, who will?

> And yes,
> this patch also fixes a possibility of live lock. (but i haven't seen actual
> live lock cause from this mistake)

hrm, I guess I'll put it in the 3.6 pile.