* [PATCH] mm: vmscam: check page order in isolating lru pages
@ 2011-12-29 12:45 Hillf Danton
2011-12-29 17:35 ` KOSAKI Motohiro
0 siblings, 1 reply; 6+ messages in thread
From: Hillf Danton @ 2011-12-29 12:45 UTC (permalink / raw)
To: linux-mm
Cc: David Rientjes, Hugh Dickins, Andrew Morton, KAMEZAWA Hiroyuki,
Michal Hocko, LKML
Before we try to isolate physically contiguous pages, check for page order is
added, and if the reclaim order is no larger than page order, we should give up
the attempt.
Signed-off-by: Hillf Danton <dhillf@gmail.com>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Hugh Dickins <hughd@google.com>
---
--- a/mm/vmscan.c Thu Dec 29 20:20:16 2011
+++ b/mm/vmscan.c Thu Dec 29 20:28:14 2011
@@ -1162,6 +1162,7 @@ static unsigned long isolate_lru_pages(u
unsigned long end_pfn;
unsigned long page_pfn;
int zone_id;
+ unsigned int isolated_pages = 0;
page = lru_to_page(src);
prefetchw_prev_lru_page(page, src, flags);
@@ -1172,7 +1173,7 @@ static unsigned long isolate_lru_pages(u
case 0:
mem_cgroup_lru_del(page);
list_move(&page->lru, dst);
- nr_taken += hpage_nr_pages(page);
+ isolated_pages = hpage_nr_pages(page);
break;
case -EBUSY:
@@ -1184,8 +1185,11 @@ static unsigned long isolate_lru_pages(u
BUG();
}
+ nr_taken += isolated_pages;
if (!order)
continue;
+ if (isolated_pages != 1 && isolated_pages >= (1 << order))
+ continue;
/*
* Attempt to take all pages in the order aligned region
@@ -1227,7 +1231,6 @@ static unsigned long isolate_lru_pages(u
break;
if (__isolate_lru_page(cursor_page, mode, file) == 0) {
- unsigned int isolated_pages;
mem_cgroup_lru_del(cursor_page);
list_move(&cursor_page->lru, dst);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: vmscam: check page order in isolating lru pages
2011-12-29 12:45 [PATCH] mm: vmscam: check page order in isolating lru pages Hillf Danton
@ 2011-12-29 17:35 ` KOSAKI Motohiro
2011-12-31 14:55 ` Hillf Danton
0 siblings, 1 reply; 6+ messages in thread
From: KOSAKI Motohiro @ 2011-12-29 17:35 UTC (permalink / raw)
To: Hillf Danton
Cc: linux-mm, David Rientjes, Hugh Dickins, Andrew Morton,
KAMEZAWA Hiroyuki, Michal Hocko, LKML
(12/29/11 7:45 AM), Hillf Danton wrote:
> Before we try to isolate physically contiguous pages, check for page order is
> added, and if the reclaim order is no larger than page order, we should give up
> the attempt.
>
> Signed-off-by: Hillf Danton<dhillf@gmail.com>
> Cc: Michal Hocko<mhocko@suse.cz>
> Cc: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Andrew Morton<akpm@linux-foundation.org>
> Cc: David Rientjes<rientjes@google.com>
> Cc: Hugh Dickins<hughd@google.com>
> ---
>
> --- a/mm/vmscan.c Thu Dec 29 20:20:16 2011
> +++ b/mm/vmscan.c Thu Dec 29 20:28:14 2011
> @@ -1162,6 +1162,7 @@ static unsigned long isolate_lru_pages(u
> unsigned long end_pfn;
> unsigned long page_pfn;
> int zone_id;
> + unsigned int isolated_pages = 0;
>
> page = lru_to_page(src);
> prefetchw_prev_lru_page(page, src, flags);
> @@ -1172,7 +1173,7 @@ static unsigned long isolate_lru_pages(u
> case 0:
> mem_cgroup_lru_del(page);
> list_move(&page->lru, dst);
> - nr_taken += hpage_nr_pages(page);
> + isolated_pages = hpage_nr_pages(page);
> break;
>
> case -EBUSY:
> @@ -1184,8 +1185,11 @@ static unsigned long isolate_lru_pages(u
> BUG();
> }
>
> + nr_taken += isolated_pages;
> if (!order)
> continue;
> + if (isolated_pages != 1&& isolated_pages>= (1<< order))
> + continue;
strange space alignment. and I don't think we need "isolated_pages != 1"
check.
Otherwise, Looks good to me.
Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: vmscam: check page order in isolating lru pages
2011-12-29 17:35 ` KOSAKI Motohiro
@ 2011-12-31 14:55 ` Hillf Danton
2012-01-04 0:05 ` Andrew Morton
2012-01-05 13:32 ` Mel Gorman
0 siblings, 2 replies; 6+ messages in thread
From: Hillf Danton @ 2011-12-31 14:55 UTC (permalink / raw)
To: KOSAKI Motohiro
Cc: linux-mm, David Rientjes, Hugh Dickins, Andrew Morton,
KAMEZAWA Hiroyuki, Michal Hocko, LKML, Andrea Arcangeli,
Mel Gorman
On Fri, Dec 30, 2011 at 1:35 AM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> wrote:
> (12/29/11 7:45 AM), Hillf Danton wrote:
>>
>> Before we try to isolate physically contiguous pages, check for page order
>> is
>> added, and if the reclaim order is no larger than page order, we should
>> give up
>> the attempt.
>>
>> Signed-off-by: Hillf Danton<dhillf@gmail.com>
>> Cc: Michal Hocko<mhocko@suse.cz>
>> Cc: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>> Cc: Andrew Morton<akpm@linux-foundation.org>
>> Cc: David Rientjes<rientjes@google.com>
>> Cc: Hugh Dickins<hughd@google.com>
>> ---
>>
>> --- a/mm/vmscan.c Thu Dec 29 20:20:16 2011
>> +++ b/mm/vmscan.c Thu Dec 29 20:28:14 2011
>> @@ -1162,6 +1162,7 @@ static unsigned long isolate_lru_pages(u
>> unsigned long end_pfn;
>> unsigned long page_pfn;
>> int zone_id;
>> + unsigned int isolated_pages = 0;
>>
>> page = lru_to_page(src);
>> prefetchw_prev_lru_page(page, src, flags);
>> @@ -1172,7 +1173,7 @@ static unsigned long isolate_lru_pages(u
>> case 0:
>> mem_cgroup_lru_del(page);
>> list_move(&page->lru, dst);
>> - nr_taken += hpage_nr_pages(page);
>> + isolated_pages = hpage_nr_pages(page);
>> break;
>>
>> case -EBUSY:
>> @@ -1184,8 +1185,11 @@ static unsigned long isolate_lru_pages(u
>> BUG();
>> }
>>
>> + nr_taken += isolated_pages;
>> if (!order)
>> continue;
>> + if (isolated_pages != 1&& isolated_pages>= (1<< order))
>> + continue;
>
>
> strange space alignment. and I don't think we need "isolated_pages != 1"
> check.
>
> Otherwise, Looks good to me.
>
> Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>
Hi KOSAKI
It is re-prepared and please review again.
1, changelog is updated,
2, the check for page order is refined,
3, comment is also added.
Thanks
Hillf
===cut please===
From: Hillf Danton <dhillf@gmail.com>
Subject: [PATCH] mm: vmscam: check page order in isolating lru pages
Before try to isolate physically contiguous pages, check for page order is
added, and if it is not regular page, we should give up the attempt.
Signed-off-by: Hillf Danton <dhillf@gmail.com>
Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Mel Gorman <mgorman@suse.de>
---
--- a/mm/vmscan.c Thu Dec 29 20:20:16 2011
+++ b/mm/vmscan.c Sat Dec 31 22:44:16 2011
@@ -1162,6 +1162,7 @@ static unsigned long isolate_lru_pages(u
unsigned long end_pfn;
unsigned long page_pfn;
int zone_id;
+ unsigned int isolated_pages = 1;
page = lru_to_page(src);
prefetchw_prev_lru_page(page, src, flags);
@@ -1172,7 +1173,7 @@ static unsigned long isolate_lru_pages(u
case 0:
mem_cgroup_lru_del(page);
list_move(&page->lru, dst);
- nr_taken += hpage_nr_pages(page);
+ isolated_pages = hpage_nr_pages(page);
break;
case -EBUSY:
@@ -1184,8 +1185,12 @@ static unsigned long isolate_lru_pages(u
BUG();
}
+ nr_taken += isolated_pages;
if (!order)
continue;
+ /* try pfn-based isolation only for regular page */
+ if (isolated_pages != 1)
+ continue;
/*
* Attempt to take all pages in the order aligned region
@@ -1227,7 +1232,6 @@ static unsigned long isolate_lru_pages(u
break;
if (__isolate_lru_page(cursor_page, mode, file) == 0) {
- unsigned int isolated_pages;
mem_cgroup_lru_del(cursor_page);
list_move(&cursor_page->lru, dst);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: vmscam: check page order in isolating lru pages
2011-12-31 14:55 ` Hillf Danton
@ 2012-01-04 0:05 ` Andrew Morton
2012-01-05 6:20 ` KAMEZAWA Hiroyuki
2012-01-05 13:32 ` Mel Gorman
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2012-01-04 0:05 UTC (permalink / raw)
To: Hillf Danton
Cc: KOSAKI Motohiro, linux-mm, David Rientjes, Hugh Dickins,
KAMEZAWA Hiroyuki, Michal Hocko, LKML, Andrea Arcangeli,
Mel Gorman
On Sat, 31 Dec 2011 22:55:22 +0800
Hillf Danton <dhillf@gmail.com> wrote:
> From: Hillf Danton <dhillf@gmail.com>
> Subject: [PATCH] mm: vmscam: check page order in isolating lru pages
>
> Before try to isolate physically contiguous pages, check for page order is
> added, and if it is not regular page, we should give up the attempt.
Well.. why? Neither the changelog nor the code comments explain why
we skip these pages. They should!
> --- a/mm/vmscan.c Thu Dec 29 20:20:16 2011
> +++ b/mm/vmscan.c Sat Dec 31 22:44:16 2011
> @@ -1162,6 +1162,7 @@ static unsigned long isolate_lru_pages(u
> unsigned long end_pfn;
> unsigned long page_pfn;
> int zone_id;
> + unsigned int isolated_pages = 1;
>
> page = lru_to_page(src);
> prefetchw_prev_lru_page(page, src, flags);
> @@ -1172,7 +1173,7 @@ static unsigned long isolate_lru_pages(u
> case 0:
> mem_cgroup_lru_del(page);
> list_move(&page->lru, dst);
> - nr_taken += hpage_nr_pages(page);
> + isolated_pages = hpage_nr_pages(page);
> break;
>
> case -EBUSY:
> @@ -1184,8 +1185,12 @@ static unsigned long isolate_lru_pages(u
> BUG();
> }
>
> + nr_taken += isolated_pages;
> if (!order)
> continue;
> + /* try pfn-based isolation only for regular page */
> + if (isolated_pages != 1)
> + continue;
>
> /*
> * Attempt to take all pages in the order aligned region
> @@ -1227,7 +1232,6 @@ static unsigned long isolate_lru_pages(u
> break;
>
> if (__isolate_lru_page(cursor_page, mode, file) == 0) {
> - unsigned int isolated_pages;
>
> mem_cgroup_lru_del(cursor_page);
> list_move(&cursor_page->lru, dst);
The code has become rather awkward.
I don't like the trick of reusing a local (isolated_pages) for other
purposes later on in the function. This introduces risk that someone
will add a usage of the local for its original application after it has
been reused. And it's a little bit deceiving for readers - they first
have to work out "oh, it's being reused for something else". It would
be better to use two identifiers. The compiler is good at reusing
registers (and sometimes stack slots) if the earlier local has gone
dead.
Also, why do we test hpage_nr_pages() here? Why not directly test
PageTransHuge()?
iow, something like this?
--- a/mm/vmscan.c~mm-vmscam-check-page-order-in-isolating-lru-pages-fix
+++ a/mm/vmscan.c
@@ -1173,7 +1173,6 @@ static unsigned long isolate_lru_pages(u
unsigned long end_pfn;
unsigned long page_pfn;
int zone_id;
- unsigned int isolated_pages = 1;
page = lru_to_page(src);
prefetchw_prev_lru_page(page, src, flags);
@@ -1184,7 +1183,7 @@ static unsigned long isolate_lru_pages(u
case 0:
mem_cgroup_lru_del(page);
list_move(&page->lru, dst);
- isolated_pages = hpage_nr_pages(page);
+ nr_taken += hpage_nr_pages(page);
break;
case -EBUSY:
@@ -1196,11 +1195,11 @@ static unsigned long isolate_lru_pages(u
BUG();
}
- nr_taken += isolated_pages;
if (!order)
continue;
- /* try pfn-based isolation only for regular page */
- if (isolated_pages != 1)
+
+ /* Try pfn-based isolation only for regular pages */
+ if (PageTransHuge(page) != 1)
continue;
/*
@@ -1243,6 +1242,7 @@ static unsigned long isolate_lru_pages(u
break;
if (__isolate_lru_page(cursor_page, mode, file) == 0) {
+ unsigned int isolated_pages;
mem_cgroup_del_lru(cursor_page);
list_move(&cursor_page->lru, dst);
If hpage_nr_pages() is the official way of testing for a thp page
then I guess this is the wrong thing to do!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: vmscam: check page order in isolating lru pages
2012-01-04 0:05 ` Andrew Morton
@ 2012-01-05 6:20 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 6+ messages in thread
From: KAMEZAWA Hiroyuki @ 2012-01-05 6:20 UTC (permalink / raw)
To: Andrew Morton
Cc: Hillf Danton, KOSAKI Motohiro, linux-mm, David Rientjes,
Hugh Dickins, Michal Hocko, LKML, Andrea Arcangeli, Mel Gorman
On Tue, 3 Jan 2012 16:05:05 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:
> On Sat, 31 Dec 2011 22:55:22 +0800
> Hillf Danton <dhillf@gmail.com> wrote:
>
> > From: Hillf Danton <dhillf@gmail.com>
> > Subject: [PATCH] mm: vmscam: check page order in isolating lru pages
> >
> > Before try to isolate physically contiguous pages, check for page order is
> > added, and if it is not regular page, we should give up the attempt.
>
> Well.. why? Neither the changelog nor the code comments explain why
> we skip these pages. They should!
>
> > --- a/mm/vmscan.c Thu Dec 29 20:20:16 2011
> > +++ b/mm/vmscan.c Sat Dec 31 22:44:16 2011
> > @@ -1162,6 +1162,7 @@ static unsigned long isolate_lru_pages(u
> > unsigned long end_pfn;
> > unsigned long page_pfn;
> > int zone_id;
> > + unsigned int isolated_pages = 1;
> >
> > page = lru_to_page(src);
> > prefetchw_prev_lru_page(page, src, flags);
> > @@ -1172,7 +1173,7 @@ static unsigned long isolate_lru_pages(u
> > case 0:
> > mem_cgroup_lru_del(page);
> > list_move(&page->lru, dst);
> > - nr_taken += hpage_nr_pages(page);
> > + isolated_pages = hpage_nr_pages(page);
> > break;
> >
> > case -EBUSY:
> > @@ -1184,8 +1185,12 @@ static unsigned long isolate_lru_pages(u
> > BUG();
> > }
> >
> > + nr_taken += isolated_pages;
> > if (!order)
> > continue;
> > + /* try pfn-based isolation only for regular page */
> > + if (isolated_pages != 1)
> > + continue;
> >
> > /*
> > * Attempt to take all pages in the order aligned region
> > @@ -1227,7 +1232,6 @@ static unsigned long isolate_lru_pages(u
> > break;
> >
> > if (__isolate_lru_page(cursor_page, mode, file) == 0) {
> > - unsigned int isolated_pages;
> >
> > mem_cgroup_lru_del(cursor_page);
> > list_move(&cursor_page->lru, dst);
>
> The code has become rather awkward.
>
> I don't like the trick of reusing a local (isolated_pages) for other
> purposes later on in the function. This introduces risk that someone
> will add a usage of the local for its original application after it has
> been reused. And it's a little bit deceiving for readers - they first
> have to work out "oh, it's being reused for something else". It would
> be better to use two identifiers. The compiler is good at reusing
> registers (and sometimes stack slots) if the earlier local has gone
> dead.
>
> Also, why do we test hpage_nr_pages() here? Why not directly test
> PageTransHuge()?
>
>
> iow, something like this?
>
> --- a/mm/vmscan.c~mm-vmscam-check-page-order-in-isolating-lru-pages-fix
> +++ a/mm/vmscan.c
> @@ -1173,7 +1173,6 @@ static unsigned long isolate_lru_pages(u
> unsigned long end_pfn;
> unsigned long page_pfn;
> int zone_id;
> - unsigned int isolated_pages = 1;
>
> page = lru_to_page(src);
> prefetchw_prev_lru_page(page, src, flags);
> @@ -1184,7 +1183,7 @@ static unsigned long isolate_lru_pages(u
> case 0:
> mem_cgroup_lru_del(page);
> list_move(&page->lru, dst);
> - isolated_pages = hpage_nr_pages(page);
> + nr_taken += hpage_nr_pages(page);
> break;
>
> case -EBUSY:
> @@ -1196,11 +1195,11 @@ static unsigned long isolate_lru_pages(u
> BUG();
> }
>
> - nr_taken += isolated_pages;
> if (!order)
> continue;
> - /* try pfn-based isolation only for regular page */
> - if (isolated_pages != 1)
> +
> + /* Try pfn-based isolation only for regular pages */
> + if (PageTransHuge(page) != 1)
> continue;
>
> /*
> @@ -1243,6 +1242,7 @@ static unsigned long isolate_lru_pages(u
> break;
>
> if (__isolate_lru_page(cursor_page, mode, file) == 0) {
> + unsigned int isolated_pages;
>
> mem_cgroup_del_lru(cursor_page);
> list_move(&cursor_page->lru, dst);
>
>
> If hpage_nr_pages() is the official way of testing for a thp page
> then I guess this is the wrong thing to do!
>
I think MAX_ORDER can be greater than THP size. So,
if (hpage_nr_pages(page) >= (1 << order))
continue;
will be good. we can remove !order check by this.
Thanks,
-Kame
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: vmscam: check page order in isolating lru pages
2011-12-31 14:55 ` Hillf Danton
2012-01-04 0:05 ` Andrew Morton
@ 2012-01-05 13:32 ` Mel Gorman
1 sibling, 0 replies; 6+ messages in thread
From: Mel Gorman @ 2012-01-05 13:32 UTC (permalink / raw)
To: Hillf Danton
Cc: KOSAKI Motohiro, linux-mm, David Rientjes, Hugh Dickins,
Andrew Morton, KAMEZAWA Hiroyuki, Michal Hocko, LKML,
Andrea Arcangeli
On Sat, Dec 31, 2011 at 10:55:22PM +0800, Hillf Danton wrote:
> From: Hillf Danton <dhillf@gmail.com>
> Subject: [PATCH] mm: vmscam: check page order in isolating lru pages
>
> Before try to isolate physically contiguous pages, check for page order is
> added, and if it is not regular page, we should give up the attempt.
>
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> Acked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Mel Gorman <mgorman@suse.de>
> ---
>
> --- a/mm/vmscan.c Thu Dec 29 20:20:16 2011
> +++ b/mm/vmscan.c Sat Dec 31 22:44:16 2011
> @@ -1162,6 +1162,7 @@ static unsigned long isolate_lru_pages(u
> unsigned long end_pfn;
> unsigned long page_pfn;
> int zone_id;
> + unsigned int isolated_pages = 1;
>
> page = lru_to_page(src);
> prefetchw_prev_lru_page(page, src, flags);
> @@ -1172,7 +1173,7 @@ static unsigned long isolate_lru_pages(u
> case 0:
> mem_cgroup_lru_del(page);
> list_move(&page->lru, dst);
> - nr_taken += hpage_nr_pages(page);
> + isolated_pages = hpage_nr_pages(page);
> break;
>
> case -EBUSY:
> @@ -1184,8 +1185,12 @@ static unsigned long isolate_lru_pages(u
> BUG();
> }
>
> + nr_taken += isolated_pages;
> if (!order)
> continue;
> + /* try pfn-based isolation only for regular page */
> + if (isolated_pages != 1)
> + continue;
>
Please put more detail in your changelogs explaining the intention
of your patch. Judging from it, this is a marginal performance
improvement when THPs are being isolated from the LRU by bypassing
lumpy reclaim.
However, basing the check on "isolated_pages" is obscure and it also
disables lumpy reclaim for the cases where order > HPAGE_SHIFT . This
is very rare (might never even happen) but it's still broken. Minimally
the check should have been something like
if (!order || isolated_pages >= (1 << order))
continue;
with a comment explaining that there is no point taking pages around
a naturally-aligned region if we just isolated a page larger than it.
This would look better, avoid reusing isolated_pages, be less obscure
and still work for cases where the requested order is larger than
a THP.
Nak to this version.
--
Mel Gorman
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-01-05 13:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-29 12:45 [PATCH] mm: vmscam: check page order in isolating lru pages Hillf Danton
2011-12-29 17:35 ` KOSAKI Motohiro
2011-12-31 14:55 ` Hillf Danton
2012-01-04 0:05 ` Andrew Morton
2012-01-05 6:20 ` KAMEZAWA Hiroyuki
2012-01-05 13:32 ` Mel Gorman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).