lkml.org 
[lkml]   [2012]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 15/16] mm: Throttle direct reclaimers if PF_MEMALLOC reserves are low and swap is backed by network storage
    On Mon, 16 Apr 2012 13:17:02 +0100
    Mel Gorman <mgorman@suse.de> 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 */
    > /*
    >
    > ...
    >


    \
     
     \ /
      Last update: 2012-05-02 01:01    [W:0.030 / U:58.804 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site