Re: [PATCH] CMA: call to putback_lru_pages

[Michal Nazarewicz <mina86@mina86.com>]

[-- 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 --]