Re: [PATCH 1/2] mm: page allocator: Adjust the per-cpu counter threshold when memory is low

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 29 Oct 2010 11:12:11 +0100
Mel Gorman <mel@csn.ul.ie> wrote:

> On Thu, Oct 28, 2010 at 03:04:33PM -0700, Andrew Morton wrote:
> > On Thu, 28 Oct 2010 16:13:35 +0100
> 
> > 
> > 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?
> > 
> 
> 112 threads CPUs 14 sockets. Workload initialisation creates NR_CPU sparse
> files that are 10*TOTAL_MEMORY/NR_CPU in size. Workload itself is NR_CPU
> processes just reading their own file.
> 
> The critical thing is the number of sockets. For single-socket-8-thread
> for example, vanilla was just 0.66% of time (although the patches did
> bring it down to 0.11%).

I'm surprised.  I thought the inefficiency here was caused by CPUs
tromping through percpu data, adding things up.  But the above info
would indicate that the problem was caused by lots of cross-socket
traffic?  If so, where did that come from?

> ...
>
> Follow-on patch?

Sometime, please.

> > >
> > > ...
> > >
> > >  				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.
> > 
> 
> Follow-on patch?
> 
> > >  				} 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?
> > 
> 
> To match the existing maximum which I assume is due to the deltas being
> stored in a s8.

hm, OK.  So (CHAR_MAX-2) would be a tad clearer, only there's no
CHAR_MAX and "2" remains mysterious ;)

I do go on about code comments a lot lately.  Eric D's kernel just
crashed because we didn't adequately comment first_zones_zonelist()
so I'm feeling all vindicated!

>
> > 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?
> > 
> 
> I asked why we couldn't move the threshold to struct zone and Christoph
> responded;
> 
> "If you move it then the cache footprint of the vm stat functions (which
> need to access the threshold for each access!) will increase and the
> performance sink dramatically. I tried to avoid placing the threshold
> there when I developed that approach but it always caused a dramatic
> regression under heavy load."

I don't really buy that.  The cache footprint will be increased by a
max of one cacheline (for zone->stat_threshold) and the cache footprint
will be actually reduced in the much larger percpu area (depending on
alignment and padding and stuff).

I'm suspecting something went wrong here, perhaps zone->stat_threshold
shared a cacheline with something unfortunate.