* [PATCH] CMA: call to putback_lru_pages @ 2012-12-17 21:25 Srinivas Pandruvada 2012-12-17 22:24 ` Michal Nazarewicz 0 siblings, 1 reply; 6+ messages in thread From: Srinivas Pandruvada @ 2012-12-17 21:25 UTC (permalink / raw) To: linux-mm; +Cc: Srinivas Pandruvada As per documentation and other places calling putback_lru_pages, on error only, except for CMA. I am not sure this is a problem for CMA or not. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- mm/page_alloc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 83637df..5a887bf 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -5802,8 +5802,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, alloc_migrate_target, 0, false, MIGRATE_SYNC); } - - putback_movable_pages(&cc->migratepages); + if (ret < 0) + putback_movable_pages(&cc->migratepages); return ret > 0 ? 0 : ret; } -- 1.7.11.7 -- 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> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] CMA: call to putback_lru_pages 2012-12-17 21:25 [PATCH] CMA: call to putback_lru_pages Srinivas Pandruvada @ 2012-12-17 22:24 ` Michal Nazarewicz 2012-12-18 9:58 ` Marek Szyprowski 2012-12-19 23:27 ` Andrew Morton 0 siblings, 2 replies; 6+ messages in thread From: Michal Nazarewicz @ 2012-12-17 22:24 UTC (permalink / raw) To: Srinivas Pandruvada, Marek Szyprowski; +Cc: linux-mm [-- Attachment #1: Type: text/plain, Size: 1390 bytes --] [+marek] On Mon, Dec 17 2012, Srinivas Pandruvada wrote: > As per documentation and other places calling putback_lru_pages, > on error only, except for CMA. I am not sure this is a problem > for CMA or not. If ret >= 0 than the list is empty anyway so the effect of this patch is to save a function call. It's also true that other callers call it only on error so __alloc_contig_migrate_range() is an odd man out here. As such: Acked-by: Michal Nazarewicz <mina86@mina86.com> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > mm/page_alloc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 83637df..5a887bf 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -5802,8 +5802,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > alloc_migrate_target, > 0, false, MIGRATE_SYNC); > } > - > - putback_movable_pages(&cc->migratepages); > + if (ret < 0) > + putback_movable_pages(&cc->migratepages); > return ret > 0 ? 0 : ret; > } -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo-- [-- Attachment #2.1: Type: text/plain, Size: 0 bytes --] [-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CMA: call to putback_lru_pages 2012-12-17 22:24 ` Michal Nazarewicz @ 2012-12-18 9:58 ` Marek Szyprowski 2012-12-19 23:27 ` Andrew Morton 1 sibling, 0 replies; 6+ messages in thread From: Marek Szyprowski @ 2012-12-18 9:58 UTC (permalink / raw) To: Michal Nazarewicz; +Cc: Srinivas Pandruvada, linux-mm Hello, On 12/17/2012 11:24 PM, Michal Nazarewicz wrote: > [+marek] > > On Mon, Dec 17 2012, Srinivas Pandruvada wrote: > > As per documentation and other places calling putback_lru_pages, > > on error only, except for CMA. I am not sure this is a problem > > for CMA or not. > > If ret >= 0 than the list is empty anyway so the effect of this patch is > to save a function call. It's also true that other callers call it only > on error so __alloc_contig_migrate_range() is an odd man out here. As > such: > > Acked-by: Michal Nazarewicz <mina86@mina86.com> Like Michal said, this is just a code cleanup without any functional change. Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > --- > > mm/page_alloc.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 83637df..5a887bf 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -5802,8 +5802,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > > alloc_migrate_target, > > 0, false, MIGRATE_SYNC); > > } > > - > > - putback_movable_pages(&cc->migratepages); > > + if (ret < 0) > > + putback_movable_pages(&cc->migratepages); > > return ret > 0 ? 0 : ret; > > } > Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CMA: call to putback_lru_pages 2012-12-17 22:24 ` Michal Nazarewicz 2012-12-18 9:58 ` Marek Szyprowski @ 2012-12-19 23:27 ` Andrew Morton 2012-12-20 15:13 ` Michal Nazarewicz 1 sibling, 1 reply; 6+ messages in thread From: Andrew Morton @ 2012-12-19 23:27 UTC (permalink / raw) To: Michal Nazarewicz; +Cc: Srinivas Pandruvada, Marek Szyprowski, linux-mm On Mon, 17 Dec 2012 23:24:14 +0100 Michal Nazarewicz <mina86@mina86.com> wrote: > [+marek] > > On Mon, Dec 17 2012, Srinivas Pandruvada wrote: > > As per documentation and other places calling putback_lru_pages, > > on error only, except for CMA. I am not sure this is a problem > > for CMA or not. > > If ret >= 0 than the list is empty anyway so the effect of this patch is > to save a function call. It's also true that other callers call it only > on error so __alloc_contig_migrate_range() is an odd man out here. As > such: > > Acked-by: Michal Nazarewicz <mina86@mina86.com> __alloc_contig_migrate_range() is a bit twisty. How does this look? From: Andrew Morton <akpm@linux-foundation.org> Subject: mm/page_alloc.c:__alloc_contig_migrate_range(): cleanup - `ret' is always zero in the we-timed-out case - remove a test-n-branch in the wrapup code Cc: Marek Szyprowski <m.szyprowski@samsung.com> Cc: Michal Nazarewicz <mina86@mina86.com> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- mm/page_alloc.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff -puN mm/page_alloc.c~mm-page_allocc-__alloc_contig_migrate_range-cleanup mm/page_alloc.c --- a/mm/page_alloc.c~mm-page_allocc-__alloc_contig_migrate_range-cleanup +++ a/mm/page_alloc.c @@ -5804,7 +5804,6 @@ static int __alloc_contig_migrate_range( } tries = 0; } else if (++tries == 5) { - ret = ret < 0 ? ret : -EBUSY; break; } @@ -5817,9 +5816,11 @@ static int __alloc_contig_migrate_range( 0, false, MIGRATE_SYNC, MR_CMA); } - if (ret < 0) + if (ret < 0) { putback_movable_pages(&cc->migratepages); - return ret > 0 ? 0 : ret; + return ret; + } + return 0; } /** _ Also, what's happening here? pfn = isolate_migratepages_range(cc->zone, cc, pfn, end, true); if (!pfn) { ret = -EINTR; break; } The isolate_migratepages_range() return value is undocumented and appears to make no sense. It returns zero if fatal_signal_pending() and if too_many_isolated&&!cc->sync. Returning -EINTR in the latter case is daft. -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CMA: call to putback_lru_pages 2012-12-19 23:27 ` Andrew Morton @ 2012-12-20 15:13 ` Michal Nazarewicz 2012-12-20 18:59 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: Michal Nazarewicz @ 2012-12-20 15:13 UTC (permalink / raw) To: Andrew Morton; +Cc: Srinivas Pandruvada, Marek Szyprowski, linux-mm [-- Attachment #1: Type: text/plain, Size: 2809 bytes --] On Thu, Dec 20 2012, Andrew Morton <akpm@linux-foundation.org> wrote: > __alloc_contig_migrate_range() is a bit twisty. How does this look? > > From: Andrew Morton <akpm@linux-foundation.org> > Subject: mm/page_alloc.c:__alloc_contig_migrate_range(): cleanup > > - `ret' is always zero in the we-timed-out case > - remove a test-n-branch in the wrapup code > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Michal Nazarewicz <mina86@mina86.com> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > --- > > mm/page_alloc.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff -puN mm/page_alloc.c~mm-page_allocc-__alloc_contig_migrate_range-cleanup mm/page_alloc.c > --- a/mm/page_alloc.c~mm-page_allocc-__alloc_contig_migrate_range-cleanup > +++ a/mm/page_alloc.c > @@ -5804,7 +5804,6 @@ static int __alloc_contig_migrate_range( > } > tries = 0; > } else if (++tries == 5) { > - ret = ret < 0 ? ret : -EBUSY; I don't really follow this change. If migration for a page failed, migrate_pages() will return a positive value, which _alloc_contig_migrate_range() must interpret as a failure, but with this change, it is possible to exit the loop after migration of some pages failed and with ret > 0 which will be interpret as success. On top of that, because ret > 0, “if (ret < 0) putback_movable_pages()” won't be executed thus pages from cc->migratepages will leak. I must be missing something here... > break; > } > > @@ -5817,9 +5816,11 @@ static int __alloc_contig_migrate_range( > 0, false, MIGRATE_SYNC, > MR_CMA); > } > - if (ret < 0) > + if (ret < 0) { > putback_movable_pages(&cc->migratepages); > - return ret > 0 ? 0 : ret; > + return ret; > + } > + return 0; > } This second hunk looks right. > > /** > _ > > > Also, what's happening here? > > pfn = isolate_migratepages_range(cc->zone, cc, > pfn, end, true); > if (!pfn) { > ret = -EINTR; > break; > } > > The isolate_migratepages_range() return value is undocumented and > appears to make no sense. It returns zero if fatal_signal_pending() > and if too_many_isolated&&!cc->sync. Returning -EINTR in the latter > case is daft. __alloc_contig_migrate_range() is always called with cc->sync == true, so the latter never happens in our case. As such, the condition terminates the loop if a fatal signal is pending. -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo-- [-- Attachment #2.1: Type: text/plain, Size: 0 bytes --] [-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] CMA: call to putback_lru_pages 2012-12-20 15:13 ` Michal Nazarewicz @ 2012-12-20 18:59 ` Andrew Morton 0 siblings, 0 replies; 6+ messages in thread From: Andrew Morton @ 2012-12-20 18:59 UTC (permalink / raw) To: Michal Nazarewicz; +Cc: Srinivas Pandruvada, Marek Szyprowski, linux-mm On Thu, 20 Dec 2012 16:13:33 +0100 Michal Nazarewicz <mina86@mina86.com> wrote: > On Thu, Dec 20 2012, Andrew Morton <akpm@linux-foundation.org> wrote: > > __alloc_contig_migrate_range() is a bit twisty. How does this look? > > > > From: Andrew Morton <akpm@linux-foundation.org> > > Subject: mm/page_alloc.c:__alloc_contig_migrate_range(): cleanup > > > > - `ret' is always zero in the we-timed-out case > > - remove a test-n-branch in the wrapup code > > > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > Cc: Michal Nazarewicz <mina86@mina86.com> > > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > > --- > > > > mm/page_alloc.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff -puN mm/page_alloc.c~mm-page_allocc-__alloc_contig_migrate_range-cleanup mm/page_alloc.c > > --- a/mm/page_alloc.c~mm-page_allocc-__alloc_contig_migrate_range-cleanup > > +++ a/mm/page_alloc.c > > @@ -5804,7 +5804,6 @@ static int __alloc_contig_migrate_range( > > } > > tries = 0; > > } else if (++tries == 5) { > > - ret = ret < 0 ? ret : -EBUSY; > > I don't really follow this change. > > If migration for a page failed, migrate_pages() will return a positive > value, which _alloc_contig_migrate_range() must interpret as a failure, > but with this change, it is possible to exit the loop after migration of > some pages failed and with ret > 0 which will be interpret as success. > > On top of that, because ret > 0, ___if (ret < 0) putback_movable_pages()___ > won't be executed thus pages from cc->migratepages will leak. I must be > missing something here... urgh, OK. > > /** > > _ > > > > > > Also, what's happening here? > > > > pfn = isolate_migratepages_range(cc->zone, cc, > > pfn, end, true); > > if (!pfn) { > > ret = -EINTR; > > break; > > } > > > > The isolate_migratepages_range() return value is undocumented and > > appears to make no sense. It returns zero if fatal_signal_pending() > > and if too_many_isolated&&!cc->sync. Returning -EINTR in the latter > > case is daft. > > __alloc_contig_migrate_range() is always called with cc->sync == true, > so the latter never happens in our case. As such, the condition > terminates the loop if a fatal signal is pending. Please prepare a patch which a) Documents the isolate_migratepages_range() return value. This documentation should mention that if isolate_migratepages_range() returns zero, the caller must again run fatal_signal_pending() to determine the reason for that zero return value. Or if that wasn't the intent then tell us what _was_ the intent. b) Explains to readers why __alloc_contig_migrate_range() isn't buggy when it assumes that a zero return from isolate_migratepages_range() means that a signal interrupted progress. But really, unless I'm missing something, the isolate_migratepages_range() return semantics are just crazy and I expect that craziness will reveal itself when you try to document it! I suspect things would be much improved if it were to return -EINTR on signal, not 0. There's a second fatal_signal_pending() check in isolate_migratepages_range() and this one can't cause a -EINTR return because the function might have made some progress. This rather forces the caller to recheck fatal_signal_pending(). If fatal_signal_pending() was true on entry, isolate_migratepages_range() might have made no progress and will return the caller's low_pfn value. In this case we could return -EINTR and thereby relieve callers from having to recheck fatal_signal_pending(), at the expense of having them call isolate_migratepages_range() a second time. Or something. It's a mess. Please, let's get some rigor and clarity in there? -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-20 18:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-17 21:25 [PATCH] CMA: call to putback_lru_pages Srinivas Pandruvada 2012-12-17 22:24 ` Michal Nazarewicz 2012-12-18 9:58 ` Marek Szyprowski 2012-12-19 23:27 ` Andrew Morton 2012-12-20 15:13 ` Michal Nazarewicz 2012-12-20 18:59 ` Andrew Morton
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).