* [PATCH 2/3] migration: Fix page corruption during hugepage migration
2011-01-20 16:17 [PATCH 1/3] When migrate_pages returns 0, all pages must have been released Minchan Kim
@ 2011-01-20 16:17 ` Minchan Kim
2011-01-20 16:17 ` [PATCH 3/3] compaction: Check migrate_pages's return value instead of list_empty Minchan Kim
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Minchan Kim @ 2011-01-20 16:17 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mm, LKML, Mel Gorman, Minchan Kim, Andrea Arcangeli
If migrate_huge_page by memory-failure fails , it calls put_page in itself
to decrease page reference and caller of migrate_huge_page also calls
putback_lru_pages. It can do double free of page so it can make page
corruption on page holder.
In addtion, clean of pages on caller is consistent behavior with
migrate_pages by cf608ac19c95804dc2df43b1f4f9e0.
Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
mm/memory-failure.c | 5 ++++-
mm/migrate.c | 4 ----
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 75398b0..237aaa4 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1295,7 +1295,10 @@ static int soft_offline_huge_page(struct page *page, int flags)
ret = migrate_huge_pages(&pagelist, new_page, MPOL_MF_MOVE_ALL, 0,
true);
if (ret) {
- putback_lru_pages(&pagelist);
+ struct page *page1, *page2;
+ list_for_each_entry_safe(page1, page2, &pagelist, lru)
+ put_page(page1);
+
pr_debug("soft offline: %#lx: migration failed %d, type %lx\n",
pfn, ret, page->flags);
if (ret > 0)
diff --git a/mm/migrate.c b/mm/migrate.c
index 7d34237..3a6d4fd 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -980,10 +980,6 @@ int migrate_huge_pages(struct list_head *from,
}
rc = 0;
out:
-
- list_for_each_entry_safe(page, page2, from, lru)
- put_page(page);
-
if (rc)
return rc;
--
1.7.0.4
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 3/3] compaction: Check migrate_pages's return value instead of list_empty
2011-01-20 16:17 [PATCH 1/3] When migrate_pages returns 0, all pages must have been released Minchan Kim
2011-01-20 16:17 ` [PATCH 2/3] migration: Fix page corruption during hugepage migration Minchan Kim
@ 2011-01-20 16:17 ` Minchan Kim
2011-01-26 8:18 ` Mel Gorman
2011-01-20 17:30 ` [PATCH 1/3] When migrate_pages returns 0, all pages must have been released Christoph Lameter
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Minchan Kim @ 2011-01-20 16:17 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mm, LKML, Mel Gorman, Minchan Kim
Many migrate_page's caller check return value instead of list_empy by
cf608ac19c95804dc2.
This patch makes compaction's migrate_pages consistent with others.
This patch should not change old behavior.
NOTE : This patch depends on [1/3].
Cc: Mel Gorman <mel@csn.ul.ie>
Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
mm/compaction.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index 6d592a0..cd0c7fc 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -497,12 +497,13 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
while ((ret = compact_finished(zone, cc)) == COMPACT_CONTINUE) {
unsigned long nr_migrate, nr_remaining;
+ int err;
if (!isolate_migratepages(zone, cc))
continue;
nr_migrate = cc->nr_migratepages;
- migrate_pages(&cc->migratepages, compaction_alloc,
+ err = migrate_pages(&cc->migratepages, compaction_alloc,
(unsigned long)cc, false,
cc->sync);
update_nr_listpages(cc);
@@ -516,7 +517,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
nr_remaining);
/* Release LRU pages not migrated */
- if (!list_empty(&cc->migratepages)) {
+ if (err) {
putback_lru_pages(&cc->migratepages);
cc->nr_migratepages = 0;
}
--
1.7.0.4
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/3] compaction: Check migrate_pages's return value instead of list_empty
2011-01-20 16:17 ` [PATCH 3/3] compaction: Check migrate_pages's return value instead of list_empty Minchan Kim
@ 2011-01-26 8:18 ` Mel Gorman
0 siblings, 0 replies; 14+ messages in thread
From: Mel Gorman @ 2011-01-26 8:18 UTC (permalink / raw)
To: Minchan Kim; +Cc: Andrew Morton, linux-mm, LKML
On Fri, Jan 21, 2011 at 01:17:07AM +0900, Minchan Kim wrote:
> Many migrate_page's caller check return value instead of list_empy by
> cf608ac19c95804dc2.
> This patch makes compaction's migrate_pages consistent with others.
> This patch should not change old behavior.
>
> NOTE : This patch depends on [1/3].
>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
I haven't tested but I am not spotting a problem;
Acked-by: Mel Gorman <mel@csn.ul.ie>
--
Mel Gorman
Linux Technology Center
IBM Dublin Software Lab
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-20 16:17 [PATCH 1/3] When migrate_pages returns 0, all pages must have been released Minchan Kim
2011-01-20 16:17 ` [PATCH 2/3] migration: Fix page corruption during hugepage migration Minchan Kim
2011-01-20 16:17 ` [PATCH 3/3] compaction: Check migrate_pages's return value instead of list_empty Minchan Kim
@ 2011-01-20 17:30 ` Christoph Lameter
2011-01-20 18:24 ` Andrea Arcangeli
2011-01-26 8:14 ` Mel Gorman
2011-01-26 23:06 ` Andrew Morton
4 siblings, 1 reply; 14+ messages in thread
From: Christoph Lameter @ 2011-01-20 17:30 UTC (permalink / raw)
To: Minchan Kim; +Cc: Andrew Morton, linux-mm, LKML, Mel Gorman, Andrea Arcangeli
On Fri, 21 Jan 2011, Minchan Kim wrote:
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 46fe8cc..7d34237 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -772,6 +772,7 @@ uncharge:
> unlock:
> unlock_page(page);
>
> +move_newpage:
> if (rc != -EAGAIN) {
> /*
> * A page that has been migrated has all references
> @@ -785,8 +786,6 @@ unlock:
> putback_lru_page(page);
> }
>
> -move_newpage:
> -
> /*
> * Move the new page to the LRU. If migration was not successful
> * then this will free the page.
>
What does this do? Not covered by the description.
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-20 17:30 ` [PATCH 1/3] When migrate_pages returns 0, all pages must have been released Christoph Lameter
@ 2011-01-20 18:24 ` Andrea Arcangeli
2011-01-20 18:49 ` Christoph Lameter
0 siblings, 1 reply; 14+ messages in thread
From: Andrea Arcangeli @ 2011-01-20 18:24 UTC (permalink / raw)
To: Christoph Lameter; +Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, Mel Gorman
Hello,
On Thu, Jan 20, 2011 at 11:30:35AM -0600, Christoph Lameter wrote:
> On Fri, 21 Jan 2011, Minchan Kim wrote:
>
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 46fe8cc..7d34237 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -772,6 +772,7 @@ uncharge:
> > unlock:
> > unlock_page(page);
> >
> > +move_newpage:
> > if (rc != -EAGAIN) {
> > /*
> > * A page that has been migrated has all references
> > @@ -785,8 +786,6 @@ unlock:
> > putback_lru_page(page);
> > }
> >
> > -move_newpage:
> > -
> > /*
> > * Move the new page to the LRU. If migration was not successful
> > * then this will free the page.
> >
>
> What does this do? Not covered by the description.
It makes a difference for the two goto move_newpage, when rc =
0. Otherwise the function will return 0, despite
putback_lru_page(page) wasn't called (and the caller of migrate_pages
won't call putback_lru_pages if migrate_pages returned 0).
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-20 18:24 ` Andrea Arcangeli
@ 2011-01-20 18:49 ` Christoph Lameter
2011-01-20 21:28 ` Andrea Arcangeli
0 siblings, 1 reply; 14+ messages in thread
From: Christoph Lameter @ 2011-01-20 18:49 UTC (permalink / raw)
To: Andrea Arcangeli; +Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, Mel Gorman
On Thu, 20 Jan 2011, Andrea Arcangeli wrote:
> Hello,
>
> On Thu, Jan 20, 2011 at 11:30:35AM -0600, Christoph Lameter wrote:
> > On Fri, 21 Jan 2011, Minchan Kim wrote:
> >
> > > diff --git a/mm/migrate.c b/mm/migrate.c
> > > index 46fe8cc..7d34237 100644
> > > --- a/mm/migrate.c
> > > +++ b/mm/migrate.c
> > > @@ -772,6 +772,7 @@ uncharge:
> > > unlock:
> > > unlock_page(page);
> > >
> > > +move_newpage:
> > > if (rc != -EAGAIN) {
> > > /*
> > > * A page that has been migrated has all references
> > > @@ -785,8 +786,6 @@ unlock:
> > > putback_lru_page(page);
> > > }
> > >
> > > -move_newpage:
> > > -
> > > /*
> > > * Move the new page to the LRU. If migration was not successful
> > > * then this will free the page.
> > >
> >
> > What does this do? Not covered by the description.
>
> It makes a difference for the two goto move_newpage, when rc =
> 0. Otherwise the function will return 0, despite
> putback_lru_page(page) wasn't called (and the caller of migrate_pages
> won't call putback_lru_pages if migrate_pages returned 0).
Think about the difference:
Moving the move_newpage will now cause another removal and freeing of the
page if rc != -EAGAIN.
The first goto move_newpage (because page count is 1) will now mean that
the page is freed twice. One because of the rc != EAGAIN branch and then
another time by the following putback_lru_page().
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-20 18:49 ` Christoph Lameter
@ 2011-01-20 21:28 ` Andrea Arcangeli
2011-01-21 16:11 ` Christoph Lameter
0 siblings, 1 reply; 14+ messages in thread
From: Andrea Arcangeli @ 2011-01-20 21:28 UTC (permalink / raw)
To: Christoph Lameter; +Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, Mel Gorman
On Thu, Jan 20, 2011 at 12:49:15PM -0600, Christoph Lameter wrote:
> On Thu, 20 Jan 2011, Andrea Arcangeli wrote:
>
> > Hello,
> >
> > On Thu, Jan 20, 2011 at 11:30:35AM -0600, Christoph Lameter wrote:
> > > On Fri, 21 Jan 2011, Minchan Kim wrote:
> > >
> > > > diff --git a/mm/migrate.c b/mm/migrate.c
> > > > index 46fe8cc..7d34237 100644
> > > > --- a/mm/migrate.c
> > > > +++ b/mm/migrate.c
> > > > @@ -772,6 +772,7 @@ uncharge:
> > > > unlock:
> > > > unlock_page(page);
> > > >
> > > > +move_newpage:
> > > > if (rc != -EAGAIN) {
> > > > /*
> > > > * A page that has been migrated has all references
> > > > @@ -785,8 +786,6 @@ unlock:
> > > > putback_lru_page(page);
> > > > }
> > > >
> > > > -move_newpage:
> > > > -
> > > > /*
> > > > * Move the new page to the LRU. If migration was not successful
> > > > * then this will free the page.
> > > >
> > >
> > > What does this do? Not covered by the description.
> >
> > It makes a difference for the two goto move_newpage, when rc =
> > 0. Otherwise the function will return 0, despite
> > putback_lru_page(page) wasn't called (and the caller of migrate_pages
> > won't call putback_lru_pages if migrate_pages returned 0).
>
> Think about the difference:
>
> Moving the move_newpage will now cause another removal and freeing of the
> page if rc != -EAGAIN.
The only ones doing "goto move_newpage" after the first two memleaks
that are fixed by this patch are always run with rc = -EAGAIN. So this
makes a difference only for the first two which were leaking memory before.
> The first goto move_newpage (because page count is 1) will now mean that
> the page is freed twice. One because of the rc != EAGAIN branch and then
> another time by the following putback_lru_page().
Which following putback_lru_page()? You mean
putback_lru_page(newpage)? That is for the newly allocated page
(allocated at the very top, so always needed), it's not relevant to
the page_count(page) = 1. The page_count 1 is hold by the caller, so
it's leaking memory right now (for everything but compaction).
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-20 21:28 ` Andrea Arcangeli
@ 2011-01-21 16:11 ` Christoph Lameter
2011-01-21 17:36 ` Andrea Arcangeli
0 siblings, 1 reply; 14+ messages in thread
From: Christoph Lameter @ 2011-01-21 16:11 UTC (permalink / raw)
To: Andrea Arcangeli; +Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, Mel Gorman
On Thu, 20 Jan 2011, Andrea Arcangeli wrote:
> Which following putback_lru_page()? You mean
> putback_lru_page(newpage)? That is for the newly allocated page
> (allocated at the very top, so always needed), it's not relevant to
> the page_count(page) = 1. The page_count 1 is hold by the caller, so
> it's leaking memory right now (for everything but compaction).
Ahh yes we removed the putback_lru_pages call from migrate_pages()
and broke the existing release logic. The caller has to call
putback_release_pages() as per commit
cf608ac19c95804dc2df43b1f4f9e068aa9034ab
If that is still the case then we still have the double free.
Could we please document the calling conventions exactly in the source?
Right now it says that the caller should call putback_lru_pages().
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-21 16:11 ` Christoph Lameter
@ 2011-01-21 17:36 ` Andrea Arcangeli
2011-01-21 23:54 ` Minchan Kim
0 siblings, 1 reply; 14+ messages in thread
From: Andrea Arcangeli @ 2011-01-21 17:36 UTC (permalink / raw)
To: Christoph Lameter; +Cc: Minchan Kim, Andrew Morton, linux-mm, LKML, Mel Gorman
On Fri, Jan 21, 2011 at 10:11:03AM -0600, Christoph Lameter wrote:
> On Thu, 20 Jan 2011, Andrea Arcangeli wrote:
>
> > Which following putback_lru_page()? You mean
> > putback_lru_page(newpage)? That is for the newly allocated page
> > (allocated at the very top, so always needed), it's not relevant to
> > the page_count(page) = 1. The page_count 1 is hold by the caller, so
> > it's leaking memory right now (for everything but compaction).
>
> Ahh yes we removed the putback_lru_pages call from migrate_pages()
> and broke the existing release logic. The caller has to call
> putback_release_pages() as per commit
putback_lru_paeges
> cf608ac19c95804dc2df43b1f4f9e068aa9034ab
That is the very commit that introduced the two bugs that I've fixed
by code review.
>
> If that is still the case then we still have the double free.
The caller only calls putback_lru_pages if ret != 0 (the two cases you
refer to happen with ret = 0).
Even if caller unconditionally calls putback_lru_pages (kind of what
compaction did), it can't double free because migrate_pages already
unlinked the pages before calling putback_lru_page(page), so there's
no way to do a double free (however if the caller unconditionally
called putback_lru_pages there would be no memleak to fix, but it
doesn't).
> Could we please document the calling conventions exactly in the source?
> Right now it says that the caller should call putback_lru_pages().
The caller should call putback_lru_pages only if ret != 0. Minchan
this is your commit we're discussing can you check the commentary?
Thanks!
Andrea
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-21 17:36 ` Andrea Arcangeli
@ 2011-01-21 23:54 ` Minchan Kim
0 siblings, 0 replies; 14+ messages in thread
From: Minchan Kim @ 2011-01-21 23:54 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Christoph Lameter, Andrew Morton, linux-mm, LKML, Mel Gorman
On Fri, Jan 21, 2011 at 06:36:18PM +0100, Andrea Arcangeli wrote:
> On Fri, Jan 21, 2011 at 10:11:03AM -0600, Christoph Lameter wrote:
> > On Thu, 20 Jan 2011, Andrea Arcangeli wrote:
> >
> > > Which following putback_lru_page()? You mean
> > > putback_lru_page(newpage)? That is for the newly allocated page
> > > (allocated at the very top, so always needed), it's not relevant to
> > > the page_count(page) = 1. The page_count 1 is hold by the caller, so
> > > it's leaking memory right now (for everything but compaction).
> >
> > Ahh yes we removed the putback_lru_pages call from migrate_pages()
> > and broke the existing release logic. The caller has to call
> > putback_release_pages() as per commit
>
> putback_lru_paeges
>
> > cf608ac19c95804dc2df43b1f4f9e068aa9034ab
>
> That is the very commit that introduced the two bugs that I've fixed
> by code review.
>
> >
> > If that is still the case then we still have the double free.
>
> The caller only calls putback_lru_pages if ret != 0 (the two cases you
> refer to happen with ret = 0).
>
> Even if caller unconditionally calls putback_lru_pages (kind of what
> compaction did), it can't double free because migrate_pages already
> unlinked the pages before calling putback_lru_page(page), so there's
> no way to do a double free (however if the caller unconditionally
> called putback_lru_pages there would be no memleak to fix, but it
> doesn't).
>
> > Could we please document the calling conventions exactly in the source?
> > Right now it says that the caller should call putback_lru_pages().
>
> The caller should call putback_lru_pages only if ret != 0. Minchan
> this is your commit we're discussing can you check the commentary?
No problem.
I will send the patch.
Thanks Adnrea, Christoph.
>
> Thanks!
> Andrea
>
--
Kind regards,
Minchan Kim
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-20 16:17 [PATCH 1/3] When migrate_pages returns 0, all pages must have been released Minchan Kim
` (2 preceding siblings ...)
2011-01-20 17:30 ` [PATCH 1/3] When migrate_pages returns 0, all pages must have been released Christoph Lameter
@ 2011-01-26 8:14 ` Mel Gorman
2011-01-26 23:06 ` Andrew Morton
4 siblings, 0 replies; 14+ messages in thread
From: Mel Gorman @ 2011-01-26 8:14 UTC (permalink / raw)
To: Minchan Kim; +Cc: Andrew Morton, linux-mm, LKML, Andrea Arcangeli
On Fri, Jan 21, 2011 at 01:17:05AM +0900, Minchan Kim wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> In some cases migrate_pages could return zero while still leaving a
> few pages in the pagelist (and some caller wouldn't notice it has to
> call putback_lru_pages after commit
> cf608ac19c95804dc2df43b1f4f9e068aa9034ab).
>
> Add one missing putback_lru_pages not added by commit
> cf608ac19c95804dc2df43b1f4f9e068aa9034ab.
>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
Better late than never;
Acked-by: Mel Gorman <mel@csn.ul.ie>
--
Mel Gorman
Linux Technology Center
IBM Dublin Software Lab
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-20 16:17 [PATCH 1/3] When migrate_pages returns 0, all pages must have been released Minchan Kim
` (3 preceding siblings ...)
2011-01-26 8:14 ` Mel Gorman
@ 2011-01-26 23:06 ` Andrew Morton
2011-01-26 23:21 ` Minchan Kim
4 siblings, 1 reply; 14+ messages in thread
From: Andrew Morton @ 2011-01-26 23:06 UTC (permalink / raw)
To: Minchan Kim; +Cc: linux-mm, LKML, Mel Gorman, Andrea Arcangeli
On Fri, 21 Jan 2011 01:17:05 +0900
Minchan Kim <minchan.kim@gmail.com> wrote:
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> In some cases migrate_pages could return zero while still leaving a
> few pages in the pagelist (and some caller wouldn't notice it has to
> call putback_lru_pages after commit
> cf608ac19c95804dc2df43b1f4f9e068aa9034ab).
>
> Add one missing putback_lru_pages not added by commit
> cf608ac19c95804dc2df43b1f4f9e068aa9034ab.
>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
Some patch administrivia:
a) you were on the delivery path for this patch, so you should have
added your signed-off-by:. I have made that change to my copy.
There's no harm in also having a Reviewed-by:, but Signed-off-by:
does imply that, we hope.
b) Andrea's From: line appeared twice.
c) Please choose patch titles which identify the subsystem which is
being patched. Plain old "mm:" will suit, although "mm:
compaction:" or "mm/compaction" would be nicer.
For some weird reason people keep on sending me patches with titles like
drivers: mmc: host: omap.c: frob the nozzle
or similar. I think there might be some documentation file which
(mis)leads them to do this. I simply do the utterly obvious and
convert it to
drivers/mmc/host/omap.c: frob the nozzle
duh.
d) Please don't identify patches via bare commit IDs. Because
commits can have different IDs in different trees. Instead use the
form cf608ac19c95 ("mm: compaction: fix COMPACTPAGEFAILED
counting"). I end up having to do this operation multiple times a
day and it's dull. And sometimes I don't even have that commit ID
in any of my trees, because they were working against some other
tree.
Also note that the 40-character commit ID has been trimmed to 12
characters or so.
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] When migrate_pages returns 0, all pages must have been released
2011-01-26 23:06 ` Andrew Morton
@ 2011-01-26 23:21 ` Minchan Kim
0 siblings, 0 replies; 14+ messages in thread
From: Minchan Kim @ 2011-01-26 23:21 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mm, LKML, Mel Gorman, Andrea Arcangeli
On Thu, Jan 27, 2011 at 8:06 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 21 Jan 2011 01:17:05 +0900
> Minchan Kim <minchan.kim@gmail.com> wrote:
>
>> From: Andrea Arcangeli <aarcange@redhat.com>
>>
>> From: Andrea Arcangeli <aarcange@redhat.com>
>>
>> In some cases migrate_pages could return zero while still leaving a
>> few pages in the pagelist (and some caller wouldn't notice it has to
>> call putback_lru_pages after commit
>> cf608ac19c95804dc2df43b1f4f9e068aa9034ab).
>>
>> Add one missing putback_lru_pages not added by commit
>> cf608ac19c95804dc2df43b1f4f9e068aa9034ab.
>>
>> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
>> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
>
> Some patch administrivia:
>
> a) you were on the delivery path for this patch, so you should have
> added your signed-off-by:. I have made that change to my copy.
>
> There's no harm in also having a Reviewed-by:, but Signed-off-by:
> does imply that, we hope.
>
> b) Andrea's From: line appeared twice.
>
> c) Please choose patch titles which identify the subsystem which is
> being patched. Plain old "mm:" will suit, although "mm:
> compaction:" or "mm/compaction" would be nicer.
>
> For some weird reason people keep on sending me patches with titles like
>
> drivers: mmc: host: omap.c: frob the nozzle
>
> or similar. I think there might be some documentation file which
> (mis)leads them to do this. I simply do the utterly obvious and
> convert it to
>
> drivers/mmc/host/omap.c: frob the nozzle
>
> duh.
>
> d) Please don't identify patches via bare commit IDs. Because
> commits can have different IDs in different trees. Instead use the
> form cf608ac19c95 ("mm: compaction: fix COMPACTPAGEFAILED
> counting"). I end up having to do this operation multiple times a
> day and it's dull. And sometimes I don't even have that commit ID
> in any of my trees, because they were working against some other
> tree.
>
> Also note that the 40-character commit ID has been trimmed to 12
> characters or so.
>
> Thanks.
>
I should have read SubmittingPatches, again.
Thanks!!
--
Kind regards,
Minchan Kim
--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 14+ messages in thread