Please revert [PATCH] user of the jiffies rounding patch: Slab

Please revert [PATCH] user of the jiffies rounding patch: Slab

[Christoph Lameter]

The slab reaper takes global locks. If one makes all cache reapers fire at 
the same time as this patch does then there will be a lot of contention 
that may result lots of interrupt holdoffs since some locks are taken
with interrupts disabled. The vm statistics counters are updated
and will content for global cachelines if this is done.

The approach is fine up to 2 cpus. With 2 cpus we can schedule one cache 
reaper on each cpu each second. So I guess that this was not noticed.

For 16 cpus we are already scheduling 8 parallel cache reapers every 
second. In a 512 cpu system desaster strikes with 256 cache reapers being 
fired off at the same time each second

Git commit 
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=2b2842146cb4105877c2be51d3857ec61ebd4ff9

This is in 2.6.20 / 2.6.21.

I'd suggest to use a staggered per cpu approach instead that runs multiple 
per cpu timers at once. But every batch of these timers must be run at an 
offset from each other. Not at the same time please.

Re: Please revert [PATCH] user of the jiffies rounding patch: Slab

[Christoph Lameter]

On Sat, 28 Apr 2007, Arjan van de Ven wrote:

> Christoph Lameter wrote:
> > The slab reaper takes global locks. If one makes all cache reapers fire at
> > the same time as this patch does then there will be a lot of contention that
> > may result lots of interrupt holdoffs since some locks are taken
> > with interrupts disabled. The vm statistics counters are updated
> > and will content for global cachelines if this is done.
> 
> > I'd suggest to use a staggered per cpu approach instead that runs multiple
> > per cpu timers at once. But every batch of these timers must be run at an
> > offset from each other. Not at the same time please.
> 
> from kernel/timer.c
> 
> 107
> 108         /*
> 109          * We don't want all cpus firing their timers at once hitting the
> 110          * same lock or cachelines, so we skew each extra cpu with an
> extra
> 111          * 3 jiffies. This 3 jiffies came originally from the mm/ code
> which
> 112          * already did this.
> 113          * The skew is done by adding 3*cpunr, then round, then subtract
> this
> 114          * extra offset again.
> 115          */
> 116         j += cpu * 3;
> 
> 
> isn't 3 jiffies distance per cpu enough for this already ??
> That's what it used to be before as well...

Hmmm... Yes but that looks like it comes from a different function. Slab
calls

 __round_jiffies_relative(HZ, cpu))

And its description says:

/**
 * __round_jiffies_relative - function to round jiffies to a full second
 * @j: the time in (relative) jiffies that should be rounded
 * @cpu: the processor number on which the timeout will happen
 *
 * __round_jiffies_relative() rounds a time delta  in the future (in jiffies)
 * up or down to (approximately) full seconds. This is useful for timers
 * for which the exact time they fire does not matter too much, as long as
 * they fire approximately every X seconds.
 *
 * By rounding these timers to whole seconds, all such timers will fire
 * at the same time, rather than at various times spread out. The goal
 * of this is to have the CPU wake up less, which saves power.
 *
 * The exact rounding is skewed for each processor to avoid all
 * processors firing at the exact same time, which could lead
 * to lock contention or spurious cache line bouncing.
 *
 * The return value is the rounded version of the @j parameter.
 */

This is exactly the wrong thing to do.

Re: Please revert [PATCH] user of the jiffies rounding patch: Slab

[Arjan van de Ven]

On Sat, 2007-04-28 at 16:35 -0700, Christoph Lameter wrote:

> Hmmm... Yes but that looks like it comes from a different function. Slab
> calls
> 
>  __round_jiffies_relative(HZ, cpu))
> 
> And its description says:
> 
> /**
>  *
>  * The exact rounding is skewed for each processor to avoid all
>  * processors firing at the exact same time, which could lead
>  * to lock contention or spurious cache line bouncing.


> This is exactly the wrong thing to do.


is it?

Re: Please revert [PATCH] user of the jiffies rounding patch: Slab

[Christoph Lameter]

On Sat, 28 Apr 2007, Arjan van de Ven wrote:

> > Hmmm... Yes but that looks like it comes from a different function. Slab
> > calls
> > 
> >  __round_jiffies_relative(HZ, cpu))
> > 
> > And its description says:
> > 
> > /**
> >  *
> >  * The exact rounding is skewed for each processor to avoid all
> >  * processors firing at the exact same time, which could lead
> >  * to lock contention or spurious cache line bouncing.
> 
> 
> > This is exactly the wrong thing to do.
> 
> 
> is it?

Umm you did not read the last bit of the description? It talks about 
firing all timers on all cpus at once.
 

Re: Please revert [PATCH] user of the jiffies rounding patch: Slab

[Arjan van de Ven]

Christoph Lameter wrote:
> Umm you did not read the last bit of the description? It talks about 
> firing all timers on all cpus at once.
>  

it talks about AVOIDING firing all timers on all cpus at once.

Re: Please revert [PATCH] user of the jiffies rounding patch: Slab

[Christoph Lameter]

On Mon, 30 Apr 2007, Arjan van de Ven wrote:

> Christoph Lameter wrote:
> > Umm you did not read the last bit of the description? It talks about firing
> > all timers on all cpus at once.
> >  
> 
> it talks about AVOIDING firing all timers on all cpus at once.

Hmmmm... Looked fine right now. Must have gotten the deep end before.

Ok. Could you review the new 1 HZ timer that I added to vmstat.c in mm and 
make sure that it does the right thing? I'd like to have that one run at 
an jiffy offset of 1 instead of 3 to give you more quiet time. 

That one is only used for SMP. With SLUB there will be no periodic timers 
at all for UP. SMP only has the vmstats update which should be quite 
lightweight compared to the slab reaper and it does not need this long 
period between them to be safe.

I'd like to have some ideas from you for NUMA. On NUMA I would like to 
have all vm statistics refresh actions on each node be synchronized (which 
would also significantly fix the situation on large NUMA for you).

I.e. on HZ the first cpus of each node run vmstats update, on HZ + 1 the 
second cpu run its etc. How could we make that work?