From: Joel Schopp <jschopp@austin.ibm.com>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@elte.hu>,
linuxppc-dev@lists.ozlabs.org,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
ego@in.ibm.com
Subject: Re: [PATCH 2/2] powerpc: implement arch_scale_smt_power for Power7
Date: Wed, 20 Jan 2010 16:36:24 -0600 [thread overview]
Message-ID: <4B578568.6080808@austin.ibm.com> (raw)
In-Reply-To: <1264023213.724.561.camel@pasglop>
>> +
>> +static inline int thread_in_smt4core(int x)
>> +{
>> + return x % 4;
>> +}
>>
>
> Needs a whitespace here though I don't really like the above. Any reason
> why you can't use the existing cpu_thread_in_core() ?
>
I will change it to cpu_thread_in_core()
>
>> +unsigned long arch_scale_smt_power(struct sched_domain *sd, int cpu)
>> +{
>> + int cpu2;
>> + int idle_count = 0;
>> +
>> + struct cpumask *cpu_map = sched_domain_span(sd);
>> +
>> + unsigned long weight = cpumask_weight(cpu_map);
>> + unsigned long smt_gain = sd->smt_gain;
>>
>
> More whitespace damage above.
>
You are better than checkpatch.pl, will fix.
>
>> + if (cpu_has_feature(CPU_FTRS_POWER7) && weight == 4) {
>> + for_each_cpu(cpu2, cpu_map) {
>> + if (idle_cpu(cpu2))
>> + idle_count++;
>> + }
>>
>
> I'm not 100% sure about the use of the CPU feature above. First I wonder
> if the right approach is to instead do something like
>
> if (!cpu_has_feature(...) !! weigth < 4)
> return default_scale_smt_power(sd, cpu);
>
> Though we may be better off using a ppc_md. hook here to avoid
> calculating the weight etc... on processors that don't need any
> of that.
>
> I also dislike your naming. I would suggest you change cpu_map to
> sibling_map() and cpu2 to sibling (or just c). One thing I wonder is how
> sure we are that sched_domain_span() is always going to give us the
> threads btw ? If we introduce another sched domain level for NUMA
> purposes can't we get confused ?
>
Right now it's 100% always giving us threads. My development version of
the patch had a BUG_ON() to check this. I expect this to stay the case
in the future as the name of the function is arch_scale_smt_power(),
which clearly denotes threads are expected.
I am not stuck on the names, I'll change it to sibling instead of cpu2
and sibling_map instead of cpu_map. It seems clear to me either way.
As for testing the ! case it seems funcationally equivalent, and mine
seems less confusing.
Having a ppc.md hook with exactly 1 user is pointless, especially since
you'll still have to calculate the weight with the ability to
dynamically disable smt.
> Also, how hot is this code path ?
>
It's every load balance, which is to say not hot, but fairly frequent.
I haven't been able to measure an impact from doing very hairy
calculations (without actually changing the weights) here vs not having
it at all in actual end workloads.
>
>> + /* the following section attempts to tweak cpu power based
>> + * on current idleness of the threads dynamically at runtime
>> + */
>> + if (idle_count == 2 || idle_count == 3 || idle_count == 4) {
>>
>
> if (idle_count > 1) ? :-)
>
Yes :) Originally I had done different weightings for each of the 3
cases, which gained in some workloads but regressed some others. But
since I'm not doing that anymore I'll fold it down to > 1
next prev parent reply other threads:[~2010-01-20 22:36 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-01-20 20:00 [PATCH 0/2] sched: arch_scale_smt_powers Joel Schopp
2010-01-20 20:02 ` [PATCH 1/2] sched: Fix the place where group powers are updated Joel Schopp
2010-01-26 23:28 ` [PATCHv2 1/2] sched: enable ARCH_POWER Joel Schopp
2010-01-28 23:20 ` [PATCHv3 " Joel Schopp
2010-02-05 20:57 ` [PATCHv4 " Joel Schopp
2010-01-20 20:04 ` [PATCH 2/2] powerpc: implement arch_scale_smt_power for Power7 Joel Schopp
2010-01-20 20:48 ` Peter Zijlstra
2010-01-20 21:58 ` Michael Neuling
2010-01-20 22:44 ` Joel Schopp
2010-01-21 8:27 ` Peter Zijlstra
2010-01-20 21:04 ` Michael Neuling
2010-01-20 22:09 ` Joel Schopp
2010-01-24 3:00 ` Benjamin Herrenschmidt
2010-01-25 17:50 ` Joel Schopp
2010-01-26 4:23 ` Benjamin Herrenschmidt
2010-01-20 21:33 ` Benjamin Herrenschmidt
2010-01-20 22:36 ` Joel Schopp [this message]
2010-01-26 23:28 ` [PATCHv2 " Joel Schopp
2010-01-27 0:52 ` Benjamin Herrenschmidt
2010-01-28 22:39 ` Joel Schopp
2010-01-29 1:23 ` Benjamin Herrenschmidt
2010-01-28 23:20 ` [PATCHv3 " Joel Schopp
2010-01-28 23:24 ` Joel Schopp
2010-01-29 1:23 ` Benjamin Herrenschmidt
2010-01-29 10:13 ` Peter Zijlstra
2010-01-29 18:34 ` Joel Schopp
2010-01-29 18:41 ` Joel Schopp
2010-02-05 20:57 ` [PATCHv4 " Joel Schopp
2010-02-14 10:12 ` Peter Zijlstra
2010-02-17 22:20 ` Michael Neuling
2010-02-18 13:17 ` Peter Zijlstra
2010-02-18 13:19 ` Peter Zijlstra
2010-02-18 16:28 ` Joel Schopp
2010-02-18 17:08 ` Peter Zijlstra
2010-02-19 6:05 ` Michael Neuling
2010-02-19 10:01 ` Peter Zijlstra
2010-02-19 11:01 ` Michael Neuling
2010-02-23 6:08 ` Michael Neuling
2010-02-23 16:24 ` Peter Zijlstra
2010-02-23 16:30 ` Peter Zijlstra
2010-02-24 6:07 ` Michael Neuling
2010-02-24 11:13 ` Michael Neuling
2010-02-24 11:58 ` Michael Neuling
2010-02-27 10:21 ` Michael Neuling
2010-03-02 14:44 ` Peter Zijlstra
2010-03-04 22:28 ` Michael Neuling
2010-01-29 12:25 ` [PATCHv3 " Gabriel Paubert
2010-01-29 16:26 ` Joel Schopp
2010-01-26 23:27 ` [PATCHv2 0/2] sched: arch_scale_smt_powers v2 Joel Schopp
2010-01-28 23:20 ` [PATCHv3 0/2] sched: arch_scale_smt_powers Joel Schopp
2010-02-05 20:57 ` [PATCHv4 " Joel Schopp
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4B578568.6080808@austin.ibm.com \
--to=jschopp@austin.ibm.com \
--cc=a.p.zijlstra@chello.nl \
--cc=benh@kernel.crashing.org \
--cc=ego@in.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mingo@elte.hu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).