Re: [PATCH 01/10] mm, compaction: do not recheck suitable_migration_target under lock

[David Rientjes <rientjes@google.com>]

On Mon, 9 Jun 2014, Vlastimil Babka wrote:

> isolate_freepages_block() rechecks if the pageblock is suitable to be a target
> for migration after it has taken the zone->lock. However, the check has been
> optimized to occur only once per pageblock, and compact_checklock_irqsave()
> might be dropping and reacquiring lock, which means somebody else might have
> changed the pageblock's migratetype meanwhile.
> 
> Furthermore, nothing prevents the migratetype to change right after
> isolate_freepages_block() has finished isolating. Given how imperfect this is,
> it's simpler to just rely on the check done in isolate_freepages() without
> lock, and not pretend that the recheck under lock guarantees anything. It is
> just a heuristic after all.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.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>
> Cc: David Rientjes <rientjes@google.com>

Acked-by: David Rientjes <rientjes@google.com>

We already do a preliminary check for suitable_migration_target() in 
isolate_freepages() in a racy way to avoid unnecessary work (and 
page_order() there is unprotected, I think you already mentioned this) so 
this seems fine to abandon.

> ---
> I suggest folding mm-compactionc-isolate_freepages_block-small-tuneup.patch into this
> 

Hmm, Andrew was just moving some code around in that patch, I'm not sure 
that it makes sense to couple these two together and your patch here is 
addressing an optimization rather than a cleanup (and you've documented it 
well, no need to obscure it with unrelated changes).