On Tue, 10 Jun 2014, Vlastimil Babka wrote:
> > > diff --git a/mm/compaction.c b/mm/compaction.c
> > > index b73b182..d37f4a8 100644
> > > --- a/mm/compaction.c
> > > +++ b/mm/compaction.c
> > > @@ -185,9 +185,14 @@ static void update_pageblock_skip(struct
> > > compact_control *cc,
> > > }
> > > #endif /* CONFIG_COMPACTION */
> > >
> > > -static inline bool should_release_lock(spinlock_t *lock)
> > > +enum compact_contended should_release_lock(spinlock_t *lock)
> > > {
> > > - return need_resched() || spin_is_contended(lock);
> > > + if (need_resched())
> > > + return COMPACT_CONTENDED_SCHED;
> > > + else if (spin_is_contended(lock))
> > > + return COMPACT_CONTENDED_LOCK;
> > > + else
> > > + return COMPACT_CONTENDED_NONE;
> > > }
> > >
> > > /*
> >
> > I think eventually we're going to remove the need_resched() heuristic
> > entirely and so enum compact_contended might be overkill, but do we need
> > to worry about spin_is_contended(lock) && need_resched() reporting
> > COMPACT_CONTENDED_SCHED here instead of COMPACT_CONTENDED_LOCK?
>
> Hm right, maybe I should reorder the two tests.
>
Yes, please.
> > > @@ -202,7 +207,9 @@ static inline bool should_release_lock(spinlock_t
> > > *lock)
> > > static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long
> > > *flags,
> > > bool locked, struct compact_control *cc)
> > > {
> > > - if (should_release_lock(lock)) {
> > > + enum compact_contended contended = should_release_lock(lock);
> > > +
> > > + if (contended) {
> > > if (locked) {
> > > spin_unlock_irqrestore(lock, *flags);
> > > locked = false;
> > > @@ -210,7 +217,7 @@ static bool compact_checklock_irqsave(spinlock_t
> > > *lock, unsigned long *flags,
> > >
> > > /* async aborts if taking too long or contended */
> > > if (cc->mode == MIGRATE_ASYNC) {
> > > - cc->contended = true;
> > > + cc->contended = contended;
> > > return false;
> > > }
> > >
> > > @@ -236,7 +243,7 @@ static inline bool compact_should_abort(struct
> > > compact_control *cc)
> > > /* async compaction aborts if contended */
> > > if (need_resched()) {
> > > if (cc->mode == MIGRATE_ASYNC) {
> > > - cc->contended = true;
> > > + cc->contended = COMPACT_CONTENDED_SCHED;
> > > return true;
> > > }
> > >
> > > @@ -1095,7 +1102,8 @@ static unsigned long compact_zone_order(struct zone
> > > *zone, int order,
> > > VM_BUG_ON(!list_empty(&cc.freepages));
> > > VM_BUG_ON(!list_empty(&cc.migratepages));
> > >
> > > - *contended = cc.contended;
> > > + /* We only signal lock contention back to the allocator */
> > > + *contended = cc.contended == COMPACT_CONTENDED_LOCK;
> > > return ret;
> > > }
> > >
> >
> > Hmm, since the only thing that matters for cc->contended is
> > COMPACT_CONTENDED_LOCK, it may make sense to just leave this as a bool
> > within struct compact_control instead of passing the actual reason around
> > when it doesn't matter.
>
> That's what I thought first. But we set cc->contended in
> isolate_freepages_block() and then check it in isolate_freepages() and
> compaction_alloc() to make sure we don't continue the free scanner once
> contention (or need_resched()) is detected. And introducing an enum, even if
> temporary measure, seemed simpler than making that checking more complex. This
> way it can stay the same once we get rid of need_resched().
>
Ok, we can always reconsider it later after need_resched() is removed.
--
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>