On Thu, 20 Dec 2012 16:13:33 +0100 Michal Nazarewicz <mina86@xxxxxxxxxx> wrote:
> 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...
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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>