Re: [PATCH 02/11] mm: compaction: introduceisolate_{free,migrate}pages_range().

[Michal Nazarewicz]

On Mon, 12 Dec 2011 17:30:52 +0100, Mel Gorman <mel@xxxxxxxxx> wrote:

> On Mon, Dec 12, 2011 at 04:22:39PM +0100, Michal Nazarewicz wrote:
> > > <SNIP>
> > >
> > >>+		if (!pfn_valid_within(pfn))
> > >>+			goto skip;
> > >
> > >The flow of this function in general with gotos of skipped and next
> > >is confusing in comparison to the existing function. For example,
> > >if this PFN is not valid, and no freelist is provided, then we call
> > >__free_page() on a PFN that is known to be invalid.
> > >
> > >>+		++nr_scanned;
> > >>+
> > >>+		if (!PageBuddy(page)) {
> > >>+skip:
> > >>+			if (freelist)
> > >>+				goto next;
> > >>+			for (; start < pfn; ++start)
> > >>+				__free_page(pfn_to_page(pfn));
> > >>+			return 0;
> > >>+		}
> > >
> > >So if a PFN is valid and !PageBuddy and no freelist is provided, we
> > >call __free_page() on it regardless of reference count. That does not
> > >sound safe.
> >
> > Sorry about that.  It's a bug in the code which was caught later on.  The
> > code should read ???__free_page(pfn_to_page(start))???.

On Mon, 12 Dec 2011 17:30:52 +0100, Mel Gorman <mel@xxxxxxxxx> wrote:
> That will call free on valid PFNs but why is it safe to call
> __free_page() at all?  You say later that CMA requires that all
> pages in the range be valid but if the pages are in use, that does
> not mean that calling __free_page() is safe. I suspect you have not
> seen a problem because the pages in the range were free as expected
> and not in use because of MIGRATE_ISOLATE.

All pages from [start, pfn) have passed through the loop body which
means that they are valid and they have been removed from buddy (for
caller's use).  Also, because of split_free_page(), all of those pages
have been split into 0-order pages.  Therefore, in error recovery, to
undo what the loop has done so far, we put give back to buddy by
calling __free_page() on each 0-order page.

> > >> 		/* Found a free page, break it into order-0 pages */
> > >> 		isolated = split_free_page(page);
> > >> 		total_isolated += isolated;
> > >>-		for (i = 0; i < isolated; i++) {
> > >>-			list_add(&page->lru, freelist);
> > >>-			page++;
> > >>+		if (freelist) {
> > >>+			struct page *p = page;
> > >>+			for (i = isolated; i; --i, ++p)
> > >>+				list_add(&p->lru, freelist);
> > >> 		}
> > >>
> > >>-		/* If a page was split, advance to the end of it */
> > >>-		if (isolated) {
> > >>-			blockpfn += isolated - 1;
> > >>-			cursor += isolated - 1;
> > >>-		}
> > >>+next:
> > >>+		pfn += isolated;
> > >>+		page += isolated;
> > >
> > >The name isolated is now confusing because it can mean either
> > >pages isolated or pages scanned depending on context. Your patch
> > >appears to be doing a lot more than is necessary to convert
> > >isolate_freepages_block into isolate_freepages_range and at this point,
> > >it's unclear why you did that.
> >
> > When CMA uses this function, it requires all pages in the range to be valid
> > and free. (Both conditions should be met but you never know.)

To be clear, I meant that the CMA expects pages to be in buddy when the function
is called but after the function finishes, all the pages in the range are removed
from buddy.  This, among other things, is why the call to split_free_page() is
necessary.

> It seems racy but I guess you are depending on MIGRATE_ISOLATE to keep
> things sane which is fine. However, I strongly suspect that if there
> is a race and a page is in use, then you will need to retry the
> migration step. Calling __free_page does not look right because
> something still has a reference to the page.
>
> > This change
> > adds a second way isolate_freepages_range() works, which is when freelist is
> > not specified, abort on invalid or non-free page, but continue as usual if
> > freelist is provided.
>
> Ok, I think you should be able to do that by not calling split_free_page
> or adding to the list if !freelist with a comment explaining why the
> pages are left on the buddy lists for the caller to figure out. Bail if
> a page-in-use is found and have the caller check that the return value
> of isolate_freepages_block == end_pfn - start_pfn.

--
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--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/