lkml.org 
[lkml]   [2011]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 02/11] mm: compaction: introduce isolate_{free,migrate}pages_range().
    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


    \
     
     \ /
      Last update: 2011-12-12 17:33    [W:0.028 / U:178.744 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site