lkml.org 
[lkml]   [2017]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 04/10] mm, page_alloc: count movable pages when stealing from pageblock
    From
    Date
    On 02/14/2017 07:10 PM, Johannes Weiner wrote:
    >
    > That makes sense to me. I have just one nit about the patch:
    >
    >> @@ -1981,10 +1994,29 @@ static void steal_suitable_fallback(struct zone *zone, struct page *page,
    >> return;
    >> }
    >>
    >> - pages = move_freepages_block(zone, page, start_type);
    >> + free_pages = move_freepages_block(zone, page, start_type,
    >> + &good_pages);
    >> + /*
    >> + * good_pages is now the number of movable pages, but if we
    >> + * want UNMOVABLE or RECLAIMABLE allocation, it's more tricky
    >> + */
    >> + if (start_type != MIGRATE_MOVABLE) {
    >> + /*
    >> + * If we are falling back to MIGRATE_MOVABLE pageblock,
    >> + * treat all non-movable pages as good. If it's UNMOVABLE
    >> + * falling back to RECLAIMABLE or vice versa, be conservative
    >> + * as we can't distinguish the exact migratetype.
    >> + */
    >> + old_block_type = get_pageblock_migratetype(page);
    >> + if (old_block_type == MIGRATE_MOVABLE)
    >> + good_pages = pageblock_nr_pages
    >> + - free_pages - good_pages;
    >
    > This line had me scratch my head for a while, and I think it's mostly
    > because of the variable naming and the way the comments are phrased.
    >
    > Could you use a variable called movable_pages to pass to and be filled
    > in by move_freepages_block?
    >
    > And instead of good_pages something like starttype_pages or
    > alike_pages or st_pages or mt_pages or something, to indicate the
    > number of pages that are comparable to the allocation's migratetype?
    >
    >> - /* Claim the whole block if over half of it is free */
    >> - if (pages >= (1 << (pageblock_order-1)) ||
    >> + /* Claim the whole block if over half of it is free or good type */
    >> + if (free_pages + good_pages >= (1 << (pageblock_order-1)) ||
    >> page_group_by_mobility_disabled)
    >> set_pageblock_migratetype(page, start_type);
    >
    > This would then read
    >
    > if (free_pages + alike_pages ...)
    >
    > which I think would be more descriptive.
    >
    > The comment leading the entire section following move_freepages_block
    > could then say something like "If a sufficient number of pages in the
    > block are either free or of comparable migratability as our
    > allocation, claim the whole block." Followed by the caveats of how we
    > determine this migratibility.
    >
    > Or maybe even the function. The comment above the function seems out
    > of date after this patch.

    I'll incorporate this for the next posting, thanks for the feedback!

    \
     
     \ /
      Last update: 2017-02-17 17:12    [W:3.046 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site