On Wed, Jan 25, 2012 at 10:27:28AM -0500, Rik van Riel wrote:

On 01/25/2012 10:00 AM, Mel Gorman wrote:

quoted

On Tue, Jan 24, 2012 at 01:21:36PM -0500, Rik van Riel wrote:

quoted

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.

quoted

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.

quoted

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

quoted

quoted

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