From: KOSAKI Motohiro <[email protected]>
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 <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Minchan Kim <[email protected]>
Cc: Wu Fengguang <[email protected]>
Cc: KAMEZAWA Hiroyuki <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
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
On 06/14/2012 12:16 PM, [email protected] wrote:
> From: KOSAKI Motohiro<[email protected]>
>
> 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<[email protected]>
> Cc: Mel Gorman<[email protected]>
> Cc: Johannes Weiner<[email protected]>
> Cc: Minchan Kim<[email protected]>
> Cc: Wu Fengguang<[email protected]>
> Cc: KAMEZAWA Hiroyuki<[email protected]>
> Cc: Rik van Riel<[email protected]>
> Cc: Andrew Morton<[email protected]>
> Signed-off-by: KOSAKI Motohiro<[email protected]>
Acked-by: Rik van Riel <[email protected]>
On 06/15/2012 01:16 AM, [email protected] wrote:
> From: KOSAKI Motohiro <[email protected]>
>
> 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 <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Minchan Kim <[email protected]>
> Cc: Wu Fengguang <[email protected]>
> Cc: KAMEZAWA Hiroyuki <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
Reviewed-by: Minchan Kim <[email protected]>
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
On Thu, Jun 14, 2012 at 12:16:10PM -0400, [email protected] wrote:
> From: KOSAKI Motohiro <[email protected]>
>
> 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 <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Minchan Kim <[email protected]>
> Cc: Wu Fengguang <[email protected]>
> Cc: KAMEZAWA Hiroyuki <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
Acked-by: Mel Gorman <[email protected]>
--
Mel Gorman
SUSE Labs
(6/15/12 3:19 AM), Minchan Kim wrote:
> On 06/15/2012 01:16 AM, [email protected] wrote:
>
>> From: KOSAKI Motohiro<[email protected]>
>>
>> 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<[email protected]>
>> Cc: Mel Gorman<[email protected]>
>> Cc: Johannes Weiner<[email protected]>
>> Cc: Minchan Kim<[email protected]>
>> Cc: Wu Fengguang<[email protected]>
>> Cc: KAMEZAWA Hiroyuki<[email protected]>
>> Cc: Rik van Riel<[email protected]>
>> Cc: Andrew Morton<[email protected]>
>> Signed-off-by: KOSAKI Motohiro<[email protected]>
>
> Reviewed-by: Minchan Kim<[email protected]>
>
> 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.
On Thu, Jun 14, 2012 at 12:16:10PM -0400, [email protected] wrote:
> From: KOSAKI Motohiro <[email protected]>
>
> 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 <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Minchan Kim <[email protected]>
> Cc: Wu Fengguang <[email protected]>
> Cc: KAMEZAWA Hiroyuki <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
Acked-by: Johannes Weiner <[email protected]>
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, [email protected] wrote:
> >
> >>From: KOSAKI Motohiro<[email protected]>
> >>
> >>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<[email protected]>
> >>Cc: Mel Gorman<[email protected]>
> >>Cc: Johannes Weiner<[email protected]>
> >>Cc: Minchan Kim<[email protected]>
> >>Cc: Wu Fengguang<[email protected]>
> >>Cc: KAMEZAWA Hiroyuki<[email protected]>
> >>Cc: Rik van Riel<[email protected]>
> >>Cc: Andrew Morton<[email protected]>
> >>Signed-off-by: KOSAKI Motohiro<[email protected]>
> >
> >Reviewed-by: Minchan Kim<[email protected]>
> >
> >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.
(2012/06/15 1:16), [email protected] wrote:
> From: KOSAKI Motohiro<[email protected]>
>
> 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<[email protected]>
> Cc: Mel Gorman<[email protected]>
> Cc: Johannes Weiner<[email protected]>
> Cc: Minchan Kim<[email protected]>
> Cc: Wu Fengguang<[email protected]>
> Cc: KAMEZAWA Hiroyuki<[email protected]>
> Cc: Rik van Riel<[email protected]>
> Cc: Andrew Morton<[email protected]>
> Signed-off-by: KOSAKI Motohiro<[email protected]>
Reviewed-by: KAMEZAWA Hiroyuki <[email protected]>
On Thu, 14 Jun 2012, [email protected] wrote:
> From: KOSAKI Motohiro <[email protected]>
>
> 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 <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Minchan Kim <[email protected]>
> Cc: Wu Fengguang <[email protected]>
> Cc: KAMEZAWA Hiroyuki <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
Acked-by: David Rientjes <[email protected]>
On Thu, 14 Jun 2012 12:16:10 -0400
[email protected] 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...
On Fri, Jun 22, 2012 at 4:19 PM, Andrew Morton
<[email protected]> wrote:
> On Thu, 14 Jun 2012 12:16:10 -0400
> [email protected] 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.
On Fri, 22 Jun 2012 17:10:59 -0400
KOSAKI Motohiro <[email protected]> wrote:
> On Fri, Jun 22, 2012 at 4:19 PM, Andrew Morton
> <[email protected]> wrote:
> > On Thu, 14 Jun 2012 12:16:10 -0400
> > [email protected] 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 [email protected]
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.