Re: [PATCH] mm: compaction: fix the page state calculation in too_many_isolated

[Vlastimil Babka]

On 01/21/2015 10:34 AM, Vinayak Menon wrote:
> Commit "3611badc1baa" (mm: vmscan: fix the page state calculation in

That appears to be a -next commit ID, which won't be the same in Linus' tree, so
it shouldn't be in commit message, AFAIK.

> too_many_isolated) fixed an issue where a number of tasks were
> blocked in reclaim path for seconds, because of vmstat_diff not being
> synced in time. A similar problem can happen in isolate_migratepages_block,
> similar calculation is performed. This patch fixes that.

I guess it's not possible to fix the stats instantly and once in the safe
versions, so that future readings will be correct without safe, right?
So until it gets fixed, each reading will have to be safe and thus expensive?

I think in case of async compaction, we could skip the safe stuff and just
terminate it - it's already done when too_many_isolated returns true, and
there's no congestion waiting in that case.

So you could extend the too_many_isolated() with "safe" parameter (as you did
for vmscan) and pass it "cc->mode != MIGRATE_ASYNC" value from
isolate_migrate_block().

> Signed-off-by: Vinayak Menon <vinmenon@xxxxxxxxxxxxxx>
> ---
>  mm/compaction.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 546e571..2d9730d 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -537,21 +537,43 @@ static void acct_isolated(struct zone *zone, struct compact_control *cc)
>  	mod_zone_page_state(zone, NR_ISOLATED_FILE, count[1]);
>  }
>  
> -/* Similar to reclaim, but different enough that they don't share logic */
> -static bool too_many_isolated(struct zone *zone)
> +static bool __too_many_isolated(struct zone *zone, int safe)
>  {
>  	unsigned long active, inactive, isolated;
>  
> -	inactive = zone_page_state(zone, NR_INACTIVE_FILE) +
> +	if (safe) {
> +		inactive = zone_page_state_snapshot(zone, NR_INACTIVE_FILE) +
> +			zone_page_state_snapshot(zone, NR_INACTIVE_ANON);
> +		active = zone_page_state_snapshot(zone, NR_ACTIVE_FILE) +
> +			zone_page_state_snapshot(zone, NR_ACTIVE_ANON);
> +		isolated = zone_page_state_snapshot(zone, NR_ISOLATED_FILE) +
> +			zone_page_state_snapshot(zone, NR_ISOLATED_ANON);
> +	} else {
> +		inactive = zone_page_state(zone, NR_INACTIVE_FILE) +
>  					zone_page_state(zone, NR_INACTIVE_ANON);

Nit: could you ident the line above (and the other 2 below) the same way as they
are in the if (safe) part?

Thanks!

> -	active = zone_page_state(zone, NR_ACTIVE_FILE) +
> +		active = zone_page_state(zone, NR_ACTIVE_FILE) +
>  					zone_page_state(zone, NR_ACTIVE_ANON);
> -	isolated = zone_page_state(zone, NR_ISOLATED_FILE) +
> +		isolated = zone_page_state(zone, NR_ISOLATED_FILE) +
>  					zone_page_state(zone, NR_ISOLATED_ANON);
> +	}
>  
>  	return isolated > (inactive + active) / 2;
>  }
>  
> +/* Similar to reclaim, but different enough that they don't share logic */
> +static bool too_many_isolated(struct zone *zone)
> +{
> +	/*
> +	 * __too_many_isolated(safe=0) is fast but inaccurate, because it
> +	 * doesn't account for the vm_stat_diff[] counters.  So if it looks
> +	 * like too_many_isolated() is about to return true, fall back to the
> +	 * slower, more accurate zone_page_state_snapshot().
> +	 */
> +	if (unlikely(__too_many_isolated(zone, 0)))
> +		return __too_many_isolated(zone, 1);
> +	return 0;
> +}
> +
>  /**
>   * isolate_migratepages_block() - isolate all migrate-able pages within
>   *				  a single pageblock
> 

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