Messages in this thread | | | Date | Mon, 12 May 2014 17:45:31 +0200 | From | Vlastimil Babka <> | Subject | Re: [PATCH] mm, compaction: properly signal and act upon lock and need_sched() contention |
| |
On 05/12/2014 05:34 PM, Naoya Horiguchi wrote: > On Mon, May 12, 2014 at 04:15:11PM +0200, Vlastimil Babka wrote: >> Compaction uses compact_checklock_irqsave() function to periodically check for >> lock contention and need_resched() to either abort async compaction, or to >> free the lock, schedule and retake the lock. When aborting, cc->contended is >> set to signal the contended state to the caller. Two problems have been >> identified in this mechanism. >> >> First, compaction also calls directly cond_resched() in both scanners when no >> lock is yet taken. This call either does not abort async compaction, or set >> cc->contended appropriately. This patch introduces a new >> compact_check_resched() function to achieve both. >> >> Second, isolate_freepages() does not check if isolate_freepages_block() >> aborted due to contention, and advances to the next pageblock. This violates >> the principle of aborting on contention, and might result in pageblocks not >> being scanned completely, since the scanning cursor is advanced. This patch >> makes isolate_freepages_block() check the cc->contended flag and abort. >> >> Reported-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >> Cc: Minchan Kim <minchan@kernel.org> >> Cc: Mel Gorman <mgorman@suse.de> >> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> Cc: Michal Nazarewicz <mina86@mina86.com> >> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> >> Cc: Christoph Lameter <cl@linux.com> >> Cc: Rik van Riel <riel@redhat.com> >> --- >> mm/compaction.c | 40 +++++++++++++++++++++++++++++++++------- >> 1 file changed, 33 insertions(+), 7 deletions(-) >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index 83ca6f9..b34ab7c 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -222,6 +222,27 @@ static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags, >> return true; >> } >> >> +/* >> + * Similar to compact_checklock_irqsave() (see its comment) for places where >> + * a zone lock is not concerned. >> + * >> + * Returns false when compaction should abort. >> + */ >> +static inline bool compact_check_resched(struct compact_control *cc) >> +{ >> + /* async compaction aborts if contended */ >> + if (need_resched()) { >> + if (cc->mode == MIGRATE_ASYNC) { >> + cc->contended = true; > > This changes the meaning of contended in struct compact_control (not just > indicating lock contention,) so please update the comment in mm/internal.h too.
It doesn't change it, since compact_checklock_irqsave() already has this semantic: if (should_release_lock(lock) && cc->mode == MIGRATE_ASYNC) cc->contended = true;
and should_release_lock() is: need_resched() || spin_is_contended(lock)
So the comment was already outdated, I will update it in v2.
> Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Thanks.
> Thanks, > Naoya Horiguchi >
| |