[lkml]   [2010]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patch in this message
    SubjectRe: [PATCH 3/8] mm: vmscan: Reclaim order-0 and use compaction instead of lumpy reclaim
    On Wed, Nov 17, 2010 at 04:22:44PM +0000, Mel Gorman wrote:
    > + */
    > + if (sc->lumpy_reclaim_mode & LUMPY_MODE_COMPACTION)
    > + nr_to_scan = max(nr_to_scan, (1UL << sc->order));

    Just one nitpick: I'm not sure this is a good idea. We can scan quite
    some pages and we may do nothing on them. First I guess for symmetry
    this should be 2UL << sc->oder to match the 2UL << order in the
    watermark checks in compaction.c (maybe it should be 3UL if something
    considering the probability at least one page is mapped and won't be
    freed is quite high). But SWAP_CLUSTER_MAX is only 32 pages.. not even
    close to 1UL << 9 (hugepage order 9). So I think this can safely be
    removed... it only makes a difference for the stack with order 2. And
    for order 2 when we take the spinlocks we can take all 32 pages
    without screwing the "free" levels in any significant way, considering
    maybe only 4 pages are really freed in the end, and if all 32 pages
    are really freed (i.e. all plain clean cache), all that matters to
    avoid freeing more cache is to stick to compaction next time around
    (i.e. at the next allocation). And if compaction fails again next time
    around, then it's all right to shrink 32 more pages even for order

    In short I'd delete the above chunk and to run the shrinker unmodified
    as this is a too lowlevel idea, and the only real life effect is to
    decrease VM scalability for kernel stack allocation a tiny bit, with
    no benefit whatsoever.

    It's subtle because the difference it'd makes it so infinitesimal and
    I can only imagine it's a worsening overall difference.
    > @@ -1425,6 +1438,9 @@ shrink_inactive_list(unsigned long nr_to_scan, struct zone *zone,
    > putback_lru_pages(zone, sc, nr_anon, nr_file, &page_list);
    > + if (sc->lumpy_reclaim_mode & LUMPY_MODE_COMPACTION)
    > + reclaimcompact_zone_order(zone, sc->order, sc->gfp_mask);
    > +
    > trace_mm_vmscan_lru_shrink_inactive(zone->zone_pgdat->node_id,
    > zone_idx(zone),
    > nr_scanned, nr_reclaimed,

    I'm worried about this one as the objective here is to increase the
    amount of free pages, and the loop won't stop until we reach
    nr_reclaimed >= nr_to_reclaim. I'm afraid it'd lead sometime to be
    doing an overwork of compaction here for no good. In short there is no
    feedback check into the loop to verify if this newly introduced
    compaction work in the shrinker lead us to get the hugepage and stop
    the loop. It sounds some pretty random compaction invocation here just
    to run it more frequently.

    nr_to_reclaim is only 32 anyway. So my suggestion is to remove it and
    let the shrinker do its thing without interleaving compaction inside
    the shrinker, without feedback check if the compaction actually
    succeeded (maybe 100% of free ram is contiguous already), and then try
    compaction again outside of the shrinker interleaving it with the
    shrinker as usual if the watermarks aren't satisfied yet after
    shrinker freed nr_to_reclaim pages.

    I prefer we keep separated the job of freeing more pages from the job
    of compacting the single free pages into higher order pages. It's only
    32 pages being freed we're talking about here so no need to calling
    compaction more frequently (if something we should increase
    nr_to_reclaim to 512 and to call compaction less frequently). If the
    interleaving of the caller isn't ok then fix it in the caller and also
    update the nr_to_reclaim, but I think keeping those separated is way
    cleaner and the mixture is unnecessary.

     \ /
      Last update: 2010-11-18 19:13    [W:0.022 / U:36.608 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site