Re: [PATCH] mm: compaction: Abort compaction if too many pages areisolated and caller is asynchronous

[Andrea Arcangeli]

On Fri, Jun 03, 2011 at 07:23:48AM +0900, Minchan Kim wrote:
> I mean we have more tail pages than head pages. So I think we are likely to
> meet tail pages. Of course, compared to all pages(page cache, anon and
> so on), compound pages would be very small percentage.

Yes that's my point, that being a small percentage it's no big deal to
break the loop early.

> > isolated the head and it's useless to insist on more tail pages (at
> > least for large page size like on x86). Plus we've compaction so
> 
> I can't understand your point. Could you elaborate it?

What I meant is that if we already isolated the head page of the THP,
we don't need to try to free the tail pages and breaking the loop
early, will still give us a chance to free a whole 2m because we
isolated the head page (it'll involve some work and swapping but if it
was a compoundtranspage we're ok to break the loop and we're not
making the logic any worse). Provided the PMD_SIZE is quite large like
2/4m...

The only way this patch makes things worse is for slub order 3 in the
process of being freed. But tail pages aren't generally free anyway so
I doubt this really makes any difference plus the tail is getting
cleared as soon as the page reaches the buddy so it's probably
unnoticeable as this then makes a difference only during a race (plus
the tail page can't be isolated, only head page can be part of lrus
and only if they're THP).

> > insisting and screwing lru ordering isn't worth it, better to be
> > permissive and abort... in fact I wouldn't dislike to remove the
> > entire lumpy logic when COMPACTION_BUILD is true, but that alters the
> > trace too...
> 
> AFAIK, it's final destination to go as compaction will not break lru
> ordering if my patch(inorder-putback) is merged.

Agreed. I like your patchset, sorry for not having reviewed it in
detail yet but there were other issues popping up in the last few
days.

> >> get_page(cursor_page)
> >> /* The page is freed already */
> >> if (1 == page_count(cursor_page)) {
> >>       put_page(cursor_page)
> >>       continue;
> >> }
> >> put_page(cursor_page);
> >
> > We can't call get_page on an tail page or we break split_huge_page,
> 
> Why don't we call get_page on tail page if tail page isn't free?
> Maybe I need investigating split_huge_page.

Yes it's split_huge_page, only gup is allowed to increase the tail
page because we're guaranteed while gup_fast does it,
split_huge_page_refcount isn't running yet, because the pmd wasn't
set as splitting and the irqs were disabled (or we'd be holding the
page_table_lock for gup slow version after checking again the pmd
wasn't splitting and so __split_huge_page_refcount will wait).

Thanks,
Andrea
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/