lkml.org 
[lkml]   [2011]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 4/4] mm: vmscan: Only read new_classzone_idx from pgdat when reclaiming successfully
    On Thu, Jun 30, 2011 at 06:05:59PM +0900, KOSAKI Motohiro wrote:
    > (2011/06/24 23:44), Mel Gorman wrote:
    > > During allocator-intensive workloads, kswapd will be woken frequently
    > > causing free memory to oscillate between the high and min watermark.
    > > This is expected behaviour. Unfortunately, if the highest zone is
    > > small, a problem occurs.
    > >
    > > When balance_pgdat() returns, it may be at a lower classzone_idx than
    > > it started because the highest zone was unreclaimable. Before checking
    > > if it should go to sleep though, it checks pgdat->classzone_idx which
    > > when there is no other activity will be MAX_NR_ZONES-1. It interprets
    > > this as it has been woken up while reclaiming, skips scheduling and
    > > reclaims again. As there is no useful reclaim work to do, it enters
    > > into a loop of shrinking slab consuming loads of CPU until the highest
    > > zone becomes reclaimable for a long period of time.
    > >
    > > There are two problems here. 1) If the returned classzone or order is
    > > lower, it'll continue reclaiming without scheduling. 2) if the highest
    > > zone was marked unreclaimable but balance_pgdat() returns immediately
    > > at DEF_PRIORITY, the new lower classzone is not communicated back to
    > > kswapd() for sleeping.
    > >
    > > This patch does two things that are related. If the end_zone is
    > > unreclaimable, this information is communicated back. Second, if
    > > the classzone or order was reduced due to failing to reclaim, new
    > > information is not read from pgdat and instead an attempt is made to go
    > > to sleep. Due to this, it is also necessary that pgdat->classzone_idx
    > > be initialised each time to pgdat->nr_zones - 1 to avoid re-reads
    > > being interpreted as wakeups.
    > >
    > > Reported-and-tested-by: Pádraig Brady <P@draigBrady.com>
    > > Signed-off-by: Mel Gorman <mgorman@suse.de>
    > > ---
    > > mm/vmscan.c | 34 +++++++++++++++++++++-------------
    > > 1 files changed, 21 insertions(+), 13 deletions(-)
    > >
    > > diff --git a/mm/vmscan.c b/mm/vmscan.c
    > > index a76b6cc2..fe854d7 100644
    > > --- a/mm/vmscan.c
    > > +++ b/mm/vmscan.c
    > > @@ -2448,7 +2448,6 @@ loop_again:
    > > if (!zone_watermark_ok_safe(zone, order,
    > > high_wmark_pages(zone), 0, 0)) {
    > > end_zone = i;
    > > - *classzone_idx = i;
    > > break;
    > > }
    > > }
    > > @@ -2528,8 +2527,11 @@ loop_again:
    > > total_scanned > sc.nr_reclaimed + sc.nr_reclaimed / 2)
    > > sc.may_writepage = 1;
    > >
    > > - if (zone->all_unreclaimable)
    > > + if (zone->all_unreclaimable) {
    > > + if (end_zone && end_zone == i)
    > > + end_zone--;
    > > continue;
    > > + }
    > >
    > > if (!zone_watermark_ok_safe(zone, order,
    > > high_wmark_pages(zone), end_zone, 0)) {
    > > @@ -2709,8 +2711,8 @@ static void kswapd_try_to_sleep(pg_data_t *pgdat, int order, int classzone_idx)
    > > */
    > > static int kswapd(void *p)
    > > {
    > > - unsigned long order;
    > > - int classzone_idx;
    > > + unsigned long order, new_order;
    > > + int classzone_idx, new_classzone_idx;
    > > pg_data_t *pgdat = (pg_data_t*)p;
    > > struct task_struct *tsk = current;
    > >
    > > @@ -2740,17 +2742,23 @@ static int kswapd(void *p)
    > > tsk->flags |= PF_MEMALLOC | PF_SWAPWRITE | PF_KSWAPD;
    > > set_freezable();
    > >
    > > - order = 0;
    > > - classzone_idx = MAX_NR_ZONES - 1;
    > > + order = new_order = 0;
    > > + classzone_idx = new_classzone_idx = pgdat->nr_zones - 1;
    > > for ( ; ; ) {
    > > - unsigned long new_order;
    > > - int new_classzone_idx;
    > > int ret;
    > >
    > > - new_order = pgdat->kswapd_max_order;
    > > - new_classzone_idx = pgdat->classzone_idx;
    > > - pgdat->kswapd_max_order = 0;
    > > - pgdat->classzone_idx = MAX_NR_ZONES - 1;
    > > + /*
    > > + * If the last balance_pgdat was unsuccessful it's unlikely a
    > > + * new request of a similar or harder type will succeed soon
    > > + * so consider going to sleep on the basis we reclaimed at
    > > + */
    > > + if (classzone_idx >= new_classzone_idx && order == new_order) {
    >
    > I'm confusing this. If we take a following scenario, new_classzone_idx may be garbage.
    >
    > 1. new_classzone_idx = pgdat->classzone_idx
    > 2. kswapd_try_to_sleep()
    > 3. classzone_idx = pgdat->classzone_idx
    > 4. balance_pgdat()
    >
    > Wouldn't we need to reinitialize new_classzone_idx nad new_order at kswapd_try_to_sleep()
    > path too?
    >

    I don't understand your question. new_classzone_idx is initialised
    before the kswapd main loop and after this patch is only updated
    only when balance_pgdat() successfully balanced but the following
    situation can arise

    1. Read for balance-request-A (order, classzone) pair
    2. Fail balance_pgdat
    3. Sleep based on (order, classzone) pair
    4. Wake for balance-request-B (order, classzone) pair where
    balance-request-B != balance-request-A
    5. Succeed balance_pgdat
    6. Compare order,classzone with balance-request-A which will treat
    balance_pgdat() as fail and try go to sleep

    This is not the same as new_classzone_idx being "garbage" but is it
    what you mean? If so, is this your proposed fix?

    diff --git a/mm/vmscan.c b/mm/vmscan.c
    index fe854d7..1a518e6 100644
    --- a/mm/vmscan.c
    +++ b/mm/vmscan.c
    @@ -2770,6 +2770,8 @@ static int kswapd(void *p)
    kswapd_try_to_sleep(pgdat, order, classzone_idx);
    order = pgdat->kswapd_max_order;
    classzone_idx = pgdat->classzone_idx;
    + new_order = order;
    + new_classzone_idx = classzone_idx;
    pgdat->kswapd_max_order = 0;
    pgdat->classzone_idx = pgdat->nr_zones - 1;
    }
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2011-06-30 12:21    [W:0.031 / U:0.676 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site