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))???.
>
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.
> >> /* 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.)
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.
> I can try and restructure this function a bit so that there are fewer ???gotos???,
> but without the above change, CMA won't really be able to use it effectively
> (it would have to provide a freelist and then validate if pages on it are
> added in order).
>
Please do and double check that __free_page logic too.
--
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
