On Thu, Dec 20 2012, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> __alloc_contig_migrate_range() is a bit twisty. How does this look?
>
> From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> Cc: Michal Nazarewicz <mina86@xxxxxxxxxx>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxx>--------------ooO--(_)--Ooo--Attachment:
pgp6DaWCdkinH.pgp
Description: PGP signature