From: Andrea Arcangeli <aarcange@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Shaohua Li <shli@kernel.org>, linux-mm@kvack.org, mgorman@suse.de
Subject: Re: [patch 1/2 v2]compaction: abort compaction loop if lock is contended or run too long
Date: Wed, 12 Sep 2012 02:48:40 +0200 [thread overview]
Message-ID: <20120912004840.GI27078@redhat.com> (raw)
In-Reply-To: <20120911163455.bb249a3c.akpm@linux-foundation.org>
On Tue, Sep 11, 2012 at 04:34:55PM -0700, Andrew Morton wrote:
> On Mon, 10 Sep 2012 09:18:30 +0800
> Shaohua Li <shli@kernel.org> wrote:
>
> > isolate_migratepages_range() might isolate none pages, for example, when
> > zone->lru_lock is contended and compaction is async. In this case, we should
> > abort compaction, otherwise, compact_zone will run a useless loop and make
> > zone->lru_lock is even contended.
> >
> > ...
> >
> > @@ -838,12 +838,14 @@ static unsigned long compact_zone_order(
> > .migratetype = allocflags_to_migratetype(gfp_mask),
> > .zone = zone,
> > .sync = sync,
> > - .contended = contended,
> > };
> > INIT_LIST_HEAD(&cc.freepages);
> > INIT_LIST_HEAD(&cc.migratepages);
> >
> > - return compact_zone(zone, &cc);
> > + ret = compact_zone(zone, &cc);
> > + if (contended)
> > + *contended = cc.contended;
> > + return ret;
> > }
> >
>
> From a quick read, `contended' is never NULL here. And defining the
"contended" pointer can be null with some caller so the if is
needed. The inner code was checking it before. This is also why we
couldn't use *contended for the loop break bugfix, because contended
could have been null at times. Now cc.contended is always available so
we can use that to fix the bug and it's self documenting as well.
> interface so that `contended' must be a valid pointer is a good change,
> IMO - it results in simpler and faster code.
Agreed.
> Alas, try_to_compact_pages()'s kerneldoc altogether forgets to describe
> this argument. Mel's
> mm-compaction-capture-a-suitable-high-order-page-immediately-when-it-is-made-available.patch
> adds a `pages' arg and forgets to document that as well.
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2012-09-12 0:48 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-10 1:18 [patch 1/2 v2]compaction: abort compaction loop if lock is contended or run too long Shaohua Li
2012-09-10 8:11 ` Mel Gorman
2012-09-11 1:45 ` Minchan Kim
2012-09-11 8:29 ` Shaohua Li
2012-09-11 8:40 ` Minchan Kim
2012-09-11 23:34 ` Andrew Morton
2012-09-12 0:48 ` Andrea Arcangeli [this message]
2012-09-12 21:20 ` Andrew Morton
2012-09-12 23:48 ` Andrea Arcangeli
2012-09-13 0:47 ` Minchan Kim
2012-09-13 2:49 ` Shaohua Li
2012-09-13 9:38 ` Mel Gorman
2012-09-13 10:13 ` Shaohua Li
2012-09-13 16:04 ` Andrea Arcangeli
2012-09-13 16:31 ` Mel Gorman
2012-09-13 17:11 ` Andrea Arcangeli
2012-09-13 10:03 ` Mel Gorman
2012-09-12 11:08 ` Mel Gorman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120912004840.GI27078@redhat.com \
--to=aarcange@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@suse.de \
--cc=shli@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).