lkml.org 
[lkml]   [2014]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v5 14/14] mm, compaction: try to capture the just-created high-order freepage
    On 07/29/2014 09:34 AM, Joonsoo Kim wrote:
    > I don't look at it in detail, but, it looks really duplicated and hard
    > to maintain. From my experience, this is really error-prone. Please
    > think of freepage counting bugs reported by my recent patchset.
    > Freepage counting handles counting at different places for performance reason
    > and finally bugs are there. IMHO, making common function and using it
    > is better than this approach even if we touch the fastpath.

    OK, so opposite opinion than Minchan's :)

    > Could you separate this patch to this patchset?
    > I think that this patch doesn't get much reviewed from other developers
    > unlike other patches.

    Yeah I will.

    >> @@ -570,6 +572,14 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
    >> unsigned long flags;
    >> bool locked = false;
    >> struct page *page = NULL, *valid_page = NULL;
    >> + unsigned long capture_pfn = 0; /* current candidate for capturing */
    >> + unsigned long next_capture_pfn = 0; /* next candidate for capturing */
    >> +
    >> + if (cc->order > 0 && cc->order <= pageblock_order && capture) {
    >> + /* This may be outside the zone, but we check that later */
    >> + capture_pfn = low_pfn & ~((1UL << cc->order) - 1);
    >> + next_capture_pfn = ALIGN(low_pfn + 1, (1UL << cc->order));
    >> + }
    >
    > Instead of inserting capture logic to common code (compaction and
    > CMA), could you add it only to compaction code such as
    > isolate_migratepages(). Capture logic needs too many hooks as you see
    > on below snippets. And it makes code so much complicated.

    Could do it in isolate_migratepages() for whole pageblocks only (as
    David's patch did), but that restricts the usefulness. Or maybe do it
    fine grained by calling isolate_migratepages_block() multiple times. But
    the overhead of multiple calls would probably suck even more for
    lower-order compactions. For CMA the added overhead is basically only
    checks for next_capture_pfn that will be always false, so predictable.
    And mostly just in branches where isolation is failing, which is not the
    CMA's "fast path" I guess?

    But I see you're talking about "complicated", not overhead. Well it's 4
    hunks inside the isolate_migratepages_block() for loop. I don't think
    it's *that* bad, thanks to how the function was cleaned up by the
    previosu patches.
    Hmm but you made me realize I could make it nicer by doing a "goto
    isolation_fail" which would handle the next_capture_pfn update at a
    single place.

    >> +static bool compact_capture_page(struct compact_control *cc)
    >> +{
    >> + struct page *page = *cc->capture_page;
    >> + int cpu;
    >> +
    >> + if (!page)
    >> + return false;
    >> +
    >> + /* Unsafe check if it's worth to try acquiring the zone->lock at all */
    >> + if (PageBuddy(page) && page_order_unsafe(page) >= cc->order)
    >> + goto try_capture;
    >> +
    >> + /*
    >> + * There's a good chance that we have just put free pages on this CPU's
    >> + * lru cache and pcplists after the page migrations. Drain them to
    >> + * allow merging.
    >> + */
    >> + cpu = get_cpu();
    >> + lru_add_drain_cpu(cpu);
    >> + drain_local_pages(NULL);
    >> + put_cpu();
    >
    > Just for curiosity.
    >
    > If lru_add_drain_cpu() is cheap enough to capture high order page, why
    > __alloc_pages_direct_compact() doesn't call it before
    > get_page_from_freelist()?

    No idea. I guess it wasn't noticed at the time that page migration uses
    putback_lru_page() on the page that was freed, which puts it into the
    lru_add cache, only to be freed. I think it would be better to free the
    page immediately in this case, and use lru_add cache only for pages that
    will really go to lru.

    Heck, it could be even better to tell page migration to skip pcplists as
    well, to avoid drain_local_pages. Often you migrate because you want to
    use the original page for something. NUMA balancing migrations are
    different, I guess.

    >> --- a/mm/page_alloc.c
    >> +++ b/mm/page_alloc.c
    >> @@ -1489,9 +1489,11 @@ static int __isolate_free_page(struct page *page, unsigned int order)
    >> {
    >> unsigned long watermark;
    >> struct zone *zone;
    >> + struct free_area *area;
    >> int mt;
    >> + unsigned int freepage_order = page_order(page);
    >>
    >> - BUG_ON(!PageBuddy(page));
    >> + VM_BUG_ON_PAGE((!PageBuddy(page) || freepage_order < order), page);
    >>
    >> zone = page_zone(page);
    >> mt = get_pageblock_migratetype(page);
    >> @@ -1506,9 +1508,12 @@ static int __isolate_free_page(struct page *page, unsigned int order)
    >> }
    >>
    >
    > In __isolate_free_page(), we check zone_watermark_ok() with order 0.
    > But normal allocation logic would check zone_watermark_ok() with requested
    > order. Your capture logic uses __isolate_free_page() and it would
    > affect compaction success rate significantly. And it means that
    > capture logic allocates high order page on page allocator
    > too aggressively compared to other component such as normal high order

    It's either that, or the extra lru drain that makes the different. But
    the "aggressiveness" would in fact mean better accuracy. Watermark
    checking may be inaccurate. Especially when memory is close to the
    watermark and there is only a single high-order page that would satisfy
    the allocation.

    > allocation. Could you test this patch again after changing order for
    > zone_watermark_ok() in __isolate_free_page()?

    I can do that. If that makes capture significantly worse, it just
    highlights the watermark checking inaccuracy.

    > Thanks.
    >



    \
     
     \ /
      Last update: 2014-07-29 18:21    [W:4.495 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site