Re: [PATCH 0/4] amd64_edac: misc fixes

Borislav Petkov
2009-06-03T18:20:48Z

On Mon, Jun 01, 2009 at 11:57:18AM -0700, H. Peter Anvin wrote:
> Borislav Petkov wrote:
> > Actually, popcnt got added to gas in July 2006 so checking the gas
> > version should suffice, IMHO.
>
> gas is part of binutils.
>
> > Anyway, I proposed something similar before but Andrew suggested that we
> > should simply slap in the opcode so we don't need the Kbuild changes.
> > The advantage of the approach is that it works unconditionally on all
> > toolchains and introduces less code changes. Hmm...
>
> That really sucks, though, in the long run. I personally prefer to have
> the "right thing" -- which in this case is probably gcc intrinsics --
> and then a fallback that will gradually fall out of use.

Ok, here's a simple performance data measurement exercise:

I went and rerouted all the cpumask_weight calls in sched.c through a
noinline local definition:

static noinline unsigned int my_weight(const struct cpumask *mask)
{
return cpumask_weight(mask);
}

so that I could be able to dynamically ftrace the invocations. Compiling
a kernel (make -j8) on a quad core Fam10h gave the following trace
(excerpt):

<idle>-0 [000] 313.120141: my_weight <-scheduler_tick
<idle>-0 [000] 313.120145: my_weight <-select_nohz_load_balancer
<idle>-0 [000] 313.124133: my_weight <-scheduler_tick
<idle>-0 [000] 313.124138: my_weight <-select_nohz_load_balancer
<idle>-0 [000] 313.128124: my_weight <-scheduler_tick
<idle>-0 [000] 313.128127: my_weight <-select_nohz_load_balancer
<idle>-0 [000] 313.132116: my_weight <-scheduler_tick
<idle>-0 [000] 313.132120: my_weight <-select_nohz_load_balancer
<idle>-0 [000] 313.136109: my_weight <-scheduler_tick
<idle>-0 [000] 313.136114: my_weight <-select_nohz_load_balancer
<...>-3986 [002] 313.138868: my_weight <-sched_balance_self
<...>-3986 [002] 313.138870: my_weight <-sched_balance_self
<...>-4064 [003] 313.138942: my_weight <-sched_balance_self
<...>-4064 [003] 313.138945: my_weight <-sched_balance_self
<...>-4064 [000] 313.142034: my_weight <-sched_balance_self
<...>-4064 [000] 313.142037: my_weight <-sched_balance_self
<...>-4065 [001] 313.143509: my_weight <-sched_balance_self
<...>-4065 [001] 313.143511: my_weight <-sched_balance_self
make-3777 [000] 313.146553: my_weight <-sched_balance_self
make-3777 [000] 313.146554: my_weight <-sched_balance_self
<...>-4066 [001] 313.146614: my_weight <-sched_balance_self
<...>-4066 [001] 313.146614: my_weight <-sched_balance_self
<...>-4066 [003] 313.149516: my_weight <-sched_balance_self

and the following stats:

compile time: ~309.373623 secs
my_weight calls on _all_ cores: 54005
(cpu0: 14262, cpu1: 14417, cpu2: 11654, cpu3: 13672)

leading to approx. 174.56 calls per second on _ALL_ cores combined. If,
hypothetically speaking, this is a representative workload and we forget
the ftrace overhead, it looks like there's no need to switch to the
hardware version of hweight since this'll bring a bunch of code changes
which simply wouldn't justify themselves wrt to performance improvement.
It is just not worth the effort.

Of course, I'm open for suggestions wrt to a better workload but from
looking at the code, the most frequent hweight call site seems to be
scheduler_tick which happens with HZ frequency and even this is by
several magnitudes not enough for a measurable performance improvement.

Hmm..?

--
Regards/Gruss,
Boris.

Operating | Advanced Micro Devices GmbH
System | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
(OSRC) | Registergericht München, HRB Nr. 43632