On Fri, Jun 24, 2016 at 04:19:35PM -0700, Andrew Morton wrote:
> On Fri, 17 Jun 2016 16:57:30 +0900 js1304@xxxxxxxxx wrote:
>
> > There was a bug reported by Sasha and minor fixes is needed
> > so I send v3.
> >
> > o fix a bg reported by Sasha (mm/compaction: split freepages
> > without holding the zone lock)
> > o add code comment for todo list (mm/page_owner: use stackdepot
> > to store stacktrace) per Michal
> > o add 'inline' keyword (mm/page_alloc: introduce post allocation
> > processing on page allocator) per Vlastimil
> > o add a patch that clean-up code per Vlastimil
>
> I've gone through v3 patches 2-9 and have plucked out the deltas to
> take what-i-had and turn that into what-you-sent. Patch 1/9 has seen a
> lot of competing churn in isolate_freepages_block(), so please review
> the current version of that, below. Between the "===" markers:
Hello, Andrew.
Below looks okay to me.
Thanks.
>
>
> static unsigned long isolate_freepages_block(struct compact_control *cc,
> unsigned long *start_pfn,
> unsigned long end_pfn,
> struct list_head *freelist,
> bool strict)
> {
> int nr_scanned = 0, total_isolated = 0;
> struct page *cursor, *valid_page = NULL;
> unsigned long flags = 0;
> bool locked = false;
> unsigned long blockpfn = *start_pfn;
> unsigned int order;
>
> cursor = pfn_to_page(blockpfn);
>
> /* Isolate free pages. */
> for (; blockpfn < end_pfn; blockpfn++, cursor++) {
> int isolated;
> struct page *page = cursor;
>
> /*
> * Periodically drop the lock (if held) regardless of its
> * contention, to give chance to IRQs. Abort if fatal signal
> * pending or async compaction detects need_resched()
> */
> if (!(blockpfn % SWAP_CLUSTER_MAX)
> && compact_unlock_should_abort(&cc->zone->lock, flags,
> &locked, cc))
> break;
>
> nr_scanned++;
> if (!pfn_valid_within(blockpfn))
> goto isolate_fail;
>
> if (!valid_page)
> valid_page = page;
>
> /*
> * For compound pages such as THP and hugetlbfs, we can save
> * potentially a lot of iterations if we skip them at once.
> * The check is racy, but we can consider only valid values
> * and the only danger is skipping too much.
> */
> if (PageCompound(page)) {
> unsigned int comp_order = compound_order(page);
>
> if (likely(comp_order < MAX_ORDER)) {
> blockpfn += (1UL << comp_order) - 1;
> cursor += (1UL << comp_order) - 1;
> }
>
> goto isolate_fail;
> }
>
> if (!PageBuddy(page))
> goto isolate_fail;
>
> ====================
> /*
> * If we already hold the lock, we can skip some rechecking.
> * Note that if we hold the lock now, checked_pageblock was
> * already set in some previous iteration (or strict is true),
> * so it is correct to skip the suitable migration target
> * recheck as well.
> */
> if (!locked) {
> /*
> * The zone lock must be held to isolate freepages.
> * Unfortunately this is a very coarse lock and can be
> * heavily contended if there are parallel allocations
> * or parallel compactions. For async compaction do not
> * spin on the lock and we acquire the lock as late as
> * possible.
> */
> locked = compact_trylock_irqsave(&cc->zone->lock,
> &flags, cc);
> if (!locked)
> break;
>
> /* Recheck this is a buddy page under lock */
> if (!PageBuddy(page))
> goto isolate_fail;
> }
>
> /* Found a free page, will break it into order-0 pages */
> order = page_order(page);
> isolated = __isolate_free_page(page, order);
> if (!isolated)
> break;
> set_page_private(page, order);
>
> total_isolated += isolated;
> cc->nr_freepages += isolated;
> list_add_tail(&page->lru, freelist);
>
> if (!strict && cc->nr_migratepages <= cc->nr_freepages) {
> blockpfn += isolated;
> break;
> }
> /* Advance to the end of split page */
> blockpfn += isolated - 1;
> cursor += isolated - 1;
> continue;
>
> isolate_fail:
> =====================
> if (strict)
> break;
> else
> continue;
>
> }
>
> if (locked)
> spin_unlock_irqrestore(&cc->zone->lock, flags);
>
> /*
> * There is a tiny chance that we have read bogus compound_order(),
> * so be careful to not go outside of the pageblock.
> */
> if (unlikely(blockpfn > end_pfn))
> blockpfn = end_pfn;
>
> trace_mm_compaction_isolate_freepages(*start_pfn, blockpfn,
> nr_scanned, total_isolated);
>
> /* Record how far we have got within the block */
> *start_pfn = blockpfn;
>
> /*
> * If strict isolation is requested by CMA then check that all the
> * pages requested were isolated. If there were any failures, 0 is
> * returned and CMA will fail.
> */
> if (strict && blockpfn < end_pfn)
> total_isolated = 0;
>
> /* Update the pageblock-skip if the whole pageblock was scanned */
> if (blockpfn == end_pfn)
> update_pageblock_skip(cc, valid_page, total_isolated, false);
>
> count_compact_events(COMPACTFREE_SCANNED, nr_scanned);
> if (total_isolated)
> count_compact_events(COMPACTISOLATED, total_isolated);
> return total_isolated;
> }
>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>