Re: [RFC PATCH 2/6] mm, compaction: skip rechecks when lock was already held

David Rientjes
2014-06-04T23:46:42Z

On Wed, 4 Jun 2014, Vlastimil Babka wrote:

> diff --git a/mm/compaction.c b/mm/compaction.c
> index f0fd4b5..27c73d7 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -332,6 +332,16 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
> goto isolate_fail;
>
> /*
> + * If we already hold the lock, we can skip some rechecking.
> + * Note that if we hold the lock now, checked_pageblock was
> + * already set in some previous iteration (or strict is true),
> + * so it is correct to skip the suitable migration target
> + * recheck as well.
> + */
> + if (locked)
> + goto skip_recheck;
> +
> + /*
> * The zone lock must be held to isolate freepages.
> * Unfortunately this is a very coarse lock and can be
> * heavily contended if there are parallel allocations
> @@ -339,9 +349,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
> * spin on the lock and we acquire the lock as late as
> * possible.
> */
> - if (!locked)
> - locked = compact_trylock_irqsave(&cc->zone->lock,
> - &flags, cc);
> + locked = compact_trylock_irqsave(&cc->zone->lock, &flags, cc);
> if (!locked)
> break;
>
> @@ -361,6 +369,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
> if (!PageBuddy(page))
> goto isolate_fail;
>
> +skip_recheck:
> /* Found a free page, break it into order-0 pages */
> isolated = split_free_page(page);
> total_isolated += isolated;

This doesn't apply cleanly, you probably need Andrew's
"mm/compaction.c:isolate_freepages_block(): small tuneup"? Rebasing the
series on -mm would probably be best.

> @@ -671,10 +680,11 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> page_count(page) > page_mapcount(page))
> continue;
>
> - /* If the lock is not held, try to take it */
> - if (!locked)
> - locked = compact_trylock_irqsave(&zone->lru_lock,
> - &flags, cc);
> + /* If we already hold the lock, we can skip some rechecking */
> + if (locked)
> + goto skip_recheck;
> +
> + locked = compact_trylock_irqsave(&zone->lru_lock, &flags, cc);
> if (!locked)
> break;
>
> @@ -686,6 +696,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
> continue;
> }
>
> +skip_recheck:
> lruvec = mem_cgroup_page_lruvec(page, zone);
>
> /* Try isolate the page */

Looks good. I wonder how the lock-taking and the rechecks would look
nested in a "if (!locked)" clause and whether that would be cleaner (and
avoid the gotos), but I assume you already looked at that.