Re: [PATCH 15/16] mm: Throttle direct reclaimers if PF_MEMALLOCreserves are low and swap is backed by network storage

[Andrew Morton]

On Mon, 16 Apr 2012 13:17:02 +0100
Mel Gorman <mgorman@xxxxxxx> wrote:

> If swap is backed by network storage such as NBD, there is a risk
> that a large number of reclaimers can hang the system by consuming
> all PF_MEMALLOC reserves. To avoid these hangs, the administrator
> must tune min_free_kbytes in advance which is a bit fragile.
> 
> This patch throttles direct reclaimers if half the PF_MEMALLOC reserves
> are in use. If the system is routinely getting throttled the system
> administrator can increase min_free_kbytes so degradation is smoother
> but the system will keep running.
> 
>
> ...
>
> +static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
> +{
> +	struct zone *zone;
> +	unsigned long pfmemalloc_reserve = 0;
> +	unsigned long free_pages = 0;
> +	int i;
> +	bool wmark_ok;
> +
> +	for (i = 0; i <= ZONE_NORMAL; i++) {
> +		zone = &pgdat->node_zones[i];
> +		pfmemalloc_reserve += min_wmark_pages(zone);
> +		free_pages += zone_page_state(zone, NR_FREE_PAGES);
> +	}
> +
> +	wmark_ok = (free_pages > pfmemalloc_reserve / 2) ? true : false;

	wmark_ok = free_pages > pfmemalloc_reserve / 2;

> +
> +	/* kswapd must be awake if processes are being throttled */
> +	if (!wmark_ok && waitqueue_active(&pgdat->kswapd_wait)) {
> +		pgdat->classzone_idx = min(pgdat->classzone_idx,
> +						(enum zone_type)ZONE_NORMAL);
> +		wake_up_interruptible(&pgdat->kswapd_wait);
> +	}
> +
> +	return wmark_ok;
> +}
> +
> +/*
> + * Throttle direct reclaimers if backing storage is backed by the network
> + * and the PFMEMALLOC reserve for the preferred node is getting dangerously
> + * depleted. kswapd will continue to make progress and wake the processes
> + * when the low watermark is reached
> + */
> +static void throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
> +					nodemask_t *nodemask)
> +{
> +	struct zone *zone;
> +	int high_zoneidx = gfp_zone(gfp_mask);
> +	pg_data_t *pgdat;
> +
> +	/* Kernel threads such as kjournald should not be throttled */

The comment should explain "why", not "what".  Particularly when the
"what" was bleedin obvious ;)

Also...   why?

> +	if (current->flags & PF_KTHREAD)
> +		return;
> +
> +	/* Check if the pfmemalloc reserves are ok */
> +	first_zones_zonelist(zonelist, high_zoneidx, NULL, &zone);
> +	pgdat = zone->zone_pgdat;
> +	if (pfmemalloc_watermark_ok(pgdat))
> +		return;
> +
> +	/*
> +	 * If the caller cannot enter the filesystem, it's possible that it
> +	 * is processing a journal transaction. In this case, it is not safe
> +	 * to block on pfmemalloc_wait as kswapd could also be blocked waiting
> +	 * to start a transaction. Instead, throttle for up to a second before
> +	 * the reclaim must continue.
> +	 */

I suppose this applies to fs locks in general, not just to
journal_start()?

> +	if (!(gfp_mask & __GFP_FS)) {
> +		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
> +			pfmemalloc_watermark_ok(pgdat), HZ);
> +		return;
> +	}
> +
> +	/* Throttle until kswapd wakes the process */
> +	wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
> +		pfmemalloc_watermark_ok(pgdat));
> +}
> +
>  unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
>  				gfp_t gfp_mask, nodemask_t *nodemask)
>  {
>
> ...
>
> @@ -2610,6 +2686,20 @@ static bool sleeping_prematurely(pg_data_t *pgdat, int order, long remaining,
>  	if (remaining)
>  		return true;
>  
> +	/*
> +	 * There is a potential race between when kswapd checks it watermarks

"its"

> +	 * and a process gets throttled. There is also a potential race if
> +	 * processes get throttled, kswapd wakes, a large process exits therby
> +	 * balancing the zones that causes kswapd to miss a wakeup. If kswapd
> +	 * is going to sleep, no process should be sleeping on pfmemalloc_wait
> +	 * so wake them now if necessary. If necessary, processes will wake
> +	 * kswapd and get throttled again
> +	 */

Yes, the possibility for missed wakeups here worried me.  There's no
synchronization and it would be easy to leave holes.

It's good that there is no timeout on the throttling - a timeout would
cover up rare races most nastily.

> +	if (waitqueue_active(&pgdat->pfmemalloc_wait)) {
> +		wake_up(&pgdat->pfmemalloc_wait);
> +		return true;
> +	}

A bool-returning function called "sleeping_prematurely" should have no
side-effects.  But it now performs wakeups.  Wanna see if there is a
way of making this nicer?

>  	/* Check the watermark levels */
>  	for (i = 0; i <= classzone_idx; i++) {
>  		struct zone *zone = pgdat->node_zones + i;
> @@ -2871,6 +2961,12 @@ loop_again:
>  			}
>  
>  		}
> +
> +		/* Wake throttled direct reclaimers if low watermark is met */

s/"what"/"why"/ !

> +		if (waitqueue_active(&pgdat->pfmemalloc_wait) &&
> +				pfmemalloc_watermark_ok(pgdat))
> +			wake_up(&pgdat->pfmemalloc_wait);
> +
>  		if (all_zones_ok || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
>  			break;		/* kswapd: all done */
>  		/*
>
> ...
>
--
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/