Re: [PATCH v2 -mm 1/3] mm: reclaim at order 0 when compaction is enabled

[Mel Gorman <mel@csn.ul.ie>]

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.
> 

Thanks.

> ><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
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>