Re: [PATCH] mm, compaction: properly signal and act upon lock and need_sched() contention

[Vlastimil Babka]

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@xxxxxxx>
> > Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
> > Cc: Minchan Kim <minchan@xxxxxxxxxx>
> > Cc: Mel Gorman <mgorman@xxxxxxx>
> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> > Cc: Michal Nazarewicz <mina86@xxxxxxxxxx>
> > Cc: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> > Cc: Christoph Lameter <cl@xxxxxxxxx>
> > Cc: Rik van Riel <riel@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxx>

Thanks.

> Thanks,
> Naoya Horiguchi

--
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/