[lkml]   [2012]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2 -mm 1/3] mm: reclaim at order 0 when compaction is enabled
On Wed, Jan 25, 2012 at 10:27:28AM -0500, Rik van Riel wrote:
> On 01/25/2012 10:00 AM, Mel Gorman wrote:
> >On Tue, Jan 24, 2012 at 01:21:36PM -0500, Rik van Riel wrote:
> >>When built with CONFIG_COMPACTION, kswapd does not try to free
> >>contiguous pages.
> >
> >balance_pgdat() gets its order from wakeup_kswapd(). This does not apply
> >to THP because kswapd does not get woken for THP but it should be woken
> >up for allocations like jumbo frames or order-1.
> In the kernel I run at home, I wake up kswapd for THP
> as well. This is a larger change, which Andrea asked
> me to delay submitting upstream for a bit.

Ok, good call. Waking kswapd up for THP is still premature.

> So far there seem to be no ill effects. I'll continue
> watching for them.
> >As kswapd does no memory compaction itself, this patch still makes
> >sense but I found the changelog misleading.
> Fair enough. I will adjust the changelog.


> ><SNIP>
> >The second effect of this change is a non-obvious side-effect. kswapd
> >will now isolate fewer pages per cycle because it will isolate
> >SWAP_CLUSTER_MAX pages instead of SWAP_CLUSTER_MAX<<order which it
> >potentially does currently. This is not wrong as such and may be
> >desirable to limit how much reclaim kswapd does but potentially it
> >impacts success rates for compaction. As this does not apply to THP,
> >it will be difficult to detect but bear in mind if we see an increase
> >in high-order allocation failures after this patch is merged. I am
> >not suggesting a change here but it would be nice to note in the
> >changelog if there is a new version of this patch.
> Good point. I am running with THP waking up kswapd, and
> things seem to behave (and compaction seems to succeed),
> but we might indeed want to change balance_pgdat to free
> more pages for higher order allocations.
> Maybe this is the place to check (in balanced_pgdat) ?
> /*
> * We do this so kswapd doesn't build up large
> priorities for
> * example when it is freeing in parallel with
> allocators. It
> * matches the direct reclaim path behaviour in
> terms of impact
> * on zone->*_priority.
> */
> if (sc.nr_reclaimed >= SWAP_CLUSTER_MAX)
> break;

It would be a good place all right. Preferably it would tie into
compaction_ready() to decide whether to continue reclaiming or not.

> >>@@ -2922,8 +2939,6 @@ out:
> >>
> >> /* If balanced, clear the congested flag */
> >> zone_clear_flag(zone, ZONE_CONGESTED);
> >>- if (i<= *classzone_idx)
> >>- balanced += zone->present_pages;
> >> }
> >
> >Why is this being deleted? It is still used by pgdat_balanced().
> This is outside of the big while loop and is not used again
> in the function.

How about here?

if (all_zones_ok || (order && pgdat_balanced(pgdat, balanced, *classzone_idx)))
break; /* kswapd: all done */

Either way, it looks like something that should be in its own patch.

> This final for loop does not appear to
> use the variable balanced at all, except for incrementing
> it.

Mel Gorman

 \ /
  Last update: 2012-01-25 17:11    [W:0.055 / U:7.592 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site