* [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).