lkml.org 
[lkml]   [2010]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] mm: page allocator: Adjust the per-cpu counter threshold when memory is low
On Thu, 28 Oct 2010 16:13:35 +0100
Mel Gorman <mel@csn.ul.ie> wrote:

> Commit [aa45484: calculate a better estimate of NR_FREE_PAGES when
> memory is low] noted that watermarks were based on the vmstat
> NR_FREE_PAGES. To avoid synchronization overhead, these counters are
> maintained on a per-cpu basis and drained both periodically and when a
> threshold is above a threshold. On large CPU systems, the difference
> between the estimate and real value of NR_FREE_PAGES can be very high.
> The system can get into a case where pages are allocated far below the
> min watermark potentially causing livelock issues. The commit solved the
> problem by taking a better reading of NR_FREE_PAGES when memory was low.
>
> Unfortately, as reported by Shaohua Li this accurate reading can consume
> a large amount of CPU time on systems with many sockets due to cache
> line bouncing. This patch takes a different approach. For large machines
> where counter drift might be unsafe and while kswapd is awake, the per-cpu
> thresholds for the target pgdat are reduced to limit the level of drift
> to what should be a safe level. This incurs a performance penalty in heavy
> memory pressure by a factor that depends on the workload and the machine but
> the machine should function correctly without accidentally exhausting all
> memory on a node. There is an additional cost when kswapd wakes and sleeps
> but the event is not expected to be frequent - in Shaohua's test case,
> there was one recorded sleep and wake event at least.
>
> To ensure that kswapd wakes up, a safe version of zone_watermark_ok()
> is introduced that takes a more accurate reading of NR_FREE_PAGES when
> called from wakeup_kswapd, when deciding whether it is really safe to go
> back to sleep in sleeping_prematurely() and when deciding if a zone is
> really balanced or not in balance_pgdat(). We are still using an expensive
> function but limiting how often it is called.

Here I go again. I have a feeling that I already said this, but I
can't find versions 2 or 3 in the archives..

Did you evaluate using plain on percpu_counters for this? They won't
solve the performance problem as they're basically the same thing as
these open-coded counters. But they'd reduce the amount of noise and
custom-coded boilerplate in mm/.

> When the test case is reproduced, the time spent in the watermark functions
> is reduced. The following report is on the percentage of time spent
> cumulatively spent in the functions zone_nr_free_pages(), zone_watermark_ok(),
> __zone_watermark_ok(), zone_watermark_ok_safe(), zone_page_state_snapshot(),
> zone_page_state().

So how did you decide which callsites needed to use the
fast-but-inaccurate zone_watermark_ok() and which needed to use the
slow-but-more-accurate zone_watermark_ok_safe()? (Those functions need
comments explaining the difference btw)


I have a feeling this problem will bite us again perhaps due to those
other callsites, but we haven't found the workload yet.

I don't undestand why restore/reduce_pgdat_percpu_threshold() were
called around that particular sleep in kswapd and nowhere else.

> vanilla 11.6615%
> disable-threshold 0.2584%

Wow. That's 12% of all CPUs? How many CPUs and what workload?

>
> ...
>
> if (!sleeping_prematurely(pgdat, order, remaining)) {
> trace_mm_vmscan_kswapd_sleep(pgdat->node_id);
> + restore_pgdat_percpu_threshold(pgdat);
> schedule();
> + reduce_pgdat_percpu_threshold(pgdat);

We could do with some code comments here explaining what's going on.

> } else {
> if (remaining)
> count_vm_event(KSWAPD_LOW_WMARK_HIT_QUICKLY);
>
> ...
>
> +static int calculate_pressure_threshold(struct zone *zone)
> +{
> + int threshold;
> + int watermark_distance;
> +
> + /*
> + * As vmstats are not up to date, there is drift between the estimated
> + * and real values. For high thresholds and a high number of CPUs, it
> + * is possible for the min watermark to be breached while the estimated
> + * value looks fine. The pressure threshold is a reduced value such
> + * that even the maximum amount of drift will not accidentally breach
> + * the min watermark
> + */
> + watermark_distance = low_wmark_pages(zone) - min_wmark_pages(zone);
> + threshold = max(1, (int)(watermark_distance / num_online_cpus()));
> +
> + /*
> + * Maximum threshold is 125

Reasoning?

> + */
> + threshold = min(125, threshold);
> +
> + return threshold;
> +}
> +
> static int calculate_threshold(struct zone *zone)
> {
> int threshold;
>
> ...
>
> +void reduce_pgdat_percpu_threshold(pg_data_t *pgdat)
> +{
> + struct zone *zone;
> + int cpu;
> + int threshold;
> + int i;
> +
> + get_online_cpus();
> + for (i = 0; i < pgdat->nr_zones; i++) {
> + zone = &pgdat->node_zones[i];
> + if (!zone->percpu_drift_mark)
> + continue;
> +
> + threshold = calculate_pressure_threshold(zone);
> + for_each_online_cpu(cpu)
> + per_cpu_ptr(zone->pageset, cpu)->stat_threshold
> + = threshold;
> + }
> + put_online_cpus();
> +}
> +
> +void restore_pgdat_percpu_threshold(pg_data_t *pgdat)
> +{
> + struct zone *zone;
> + int cpu;
> + int threshold;
> + int i;
> +
> + get_online_cpus();
> + for (i = 0; i < pgdat->nr_zones; i++) {
> + zone = &pgdat->node_zones[i];
> + if (!zone->percpu_drift_mark)
> + continue;
> +
> + threshold = calculate_threshold(zone);
> + for_each_online_cpu(cpu)
> + per_cpu_ptr(zone->pageset, cpu)->stat_threshold
> + = threshold;
> + }
> + put_online_cpus();
> +}

Given that ->stat_threshold is the same for each CPU, why store it for
each CPU at all? Why not put it in the zone and eliminate the inner
loop?




\
 
 \ /
  Last update: 2010-10-29 00:07    [from the cache]
©2003-2011 Jasper Spaans