From: Benjamin Herrenschmidt <benh@kernel.crashing.org> To: Joel Schopp <jschopp@austin.ibm.com> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>, ego@in.ibm.com, linuxppc-dev@lists.ozlabs.org, Ingo Molnar <mingo@elte.hu>, linux-kernel@vger.kernel.org Subject: Re: [PATCHv2 2/2] powerpc: implement arch_scale_smt_power for Power7 Date: Wed, 27 Jan 2010 11:52:57 +1100 [thread overview] Message-ID: <1264553577.3601.144.camel@pasglop> (raw) In-Reply-To: <1264548495.12239.56.camel@jschopp-laptop> On Tue, 2010-01-26 at 17:28 -0600, Joel Schopp wrote: > On Power7 processors running in SMT4 mode with 2, 3, or 4 idle threads > there is performance benefit to idling the higher numbered threads in > the core. > > This patch implements arch_scale_smt_power to dynamically update smt > thread power in these idle cases in order to prefer threads 0,1 over > threads 2,3 within a core. > > v2 - Same functionality as v1, better coding style. Better. Some more comments... > Signed-off-by: Joel Schopp <jschopp@austin.ibm.com> > --- > Version 2 addresses style and optimization, same basic functionality > Index: linux-2.6.git/arch/powerpc/kernel/smp.c > =================================================================== > --- linux-2.6.git.orig/arch/powerpc/kernel/smp.c > +++ linux-2.6.git/arch/powerpc/kernel/smp.c > @@ -620,3 +620,55 @@ void __cpu_die(unsigned int cpu) > smp_ops->cpu_die(cpu); > } > #endif > + > +unsigned long arch_scale_smt_power(struct sched_domain *sd, int cpu) > +{ > + int sibling; > + int idle_count = 0; > + int thread; > + > + struct cpumask *sibling_map = sched_domain_span(sd); What about an early exit if !cpu_has_feature(CPU_FTR_SMT) ? That would de-facto compile it out for 32-bit CPU platforms that don't support SMT at all and avoid some overhead on POWER3,4,970... > + unsigned long weight = cpumask_weight(sibling_map); > + unsigned long smt_gain = sd->smt_gain; > + > + if (cpu_has_feature(CPU_FTR_ASYNC_SMT4) && weight == 4) { So that will only handle the case where all 4 threads are online right ? There is no provision for the case where the user play tricks like offlining thread, in which case it will stop trying to "push down" processes right ? Not a big deal per-se I suppose, just something to be aware of. Also, can you add a comment as to why this is done in the code itself ? above the if (cpu_has_feature(...)) statement. > + for_each_cpu(sibling, sibling_map) { > + if (idle_cpu(sibling)) > + idle_count++; > + } > + > + /* the following section attempts to tweak cpu power based > + * on current idleness of the threads dynamically at runtime > + */ > + if (idle_count > 1) { > + thread = cpu_thread_in_core(cpu); > + if (thread < 2) { > + /* add 75 % to thread power */ > + smt_gain += (smt_gain >> 1) + (smt_gain >> 2); > + } else { > + /* subtract 75 % to thread power */ > + smt_gain = smt_gain >> 2; > + } > + } > + } > + > + /* default smt gain is 1178, weight is # of SMT threads */ > + switch (weight) { > + case 1: > + /*divide by 1, do nothing*/ > + break; > + case 2: > + smt_gain = smt_gain >> 1; > + break; > + case 4: > + smt_gain = smt_gain >> 2; > + break; > + default: > + smt_gain /= weight; > + break; > + } > + > + return smt_gain; > +} Appart from that, it looks allright to me. Cheers, Ben.
WARNING: multiple messages have this Message-ID (diff)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> To: Joel Schopp <jschopp@austin.ibm.com> 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: [PATCHv2 2/2] powerpc: implement arch_scale_smt_power for Power7 Date: Wed, 27 Jan 2010 11:52:57 +1100 [thread overview] Message-ID: <1264553577.3601.144.camel@pasglop> (raw) In-Reply-To: <1264548495.12239.56.camel@jschopp-laptop> On Tue, 2010-01-26 at 17:28 -0600, Joel Schopp wrote: > On Power7 processors running in SMT4 mode with 2, 3, or 4 idle threads > there is performance benefit to idling the higher numbered threads in > the core. > > This patch implements arch_scale_smt_power to dynamically update smt > thread power in these idle cases in order to prefer threads 0,1 over > threads 2,3 within a core. > > v2 - Same functionality as v1, better coding style. Better. Some more comments... > Signed-off-by: Joel Schopp <jschopp@austin.ibm.com> > --- > Version 2 addresses style and optimization, same basic functionality > Index: linux-2.6.git/arch/powerpc/kernel/smp.c > =================================================================== > --- linux-2.6.git.orig/arch/powerpc/kernel/smp.c > +++ linux-2.6.git/arch/powerpc/kernel/smp.c > @@ -620,3 +620,55 @@ void __cpu_die(unsigned int cpu) > smp_ops->cpu_die(cpu); > } > #endif > + > +unsigned long arch_scale_smt_power(struct sched_domain *sd, int cpu) > +{ > + int sibling; > + int idle_count = 0; > + int thread; > + > + struct cpumask *sibling_map = sched_domain_span(sd); What about an early exit if !cpu_has_feature(CPU_FTR_SMT) ? That would de-facto compile it out for 32-bit CPU platforms that don't support SMT at all and avoid some overhead on POWER3,4,970... > + unsigned long weight = cpumask_weight(sibling_map); > + unsigned long smt_gain = sd->smt_gain; > + > + if (cpu_has_feature(CPU_FTR_ASYNC_SMT4) && weight == 4) { So that will only handle the case where all 4 threads are online right ? There is no provision for the case where the user play tricks like offlining thread, in which case it will stop trying to "push down" processes right ? Not a big deal per-se I suppose, just something to be aware of. Also, can you add a comment as to why this is done in the code itself ? above the if (cpu_has_feature(...)) statement. > + for_each_cpu(sibling, sibling_map) { > + if (idle_cpu(sibling)) > + idle_count++; > + } > + > + /* the following section attempts to tweak cpu power based > + * on current idleness of the threads dynamically at runtime > + */ > + if (idle_count > 1) { > + thread = cpu_thread_in_core(cpu); > + if (thread < 2) { > + /* add 75 % to thread power */ > + smt_gain += (smt_gain >> 1) + (smt_gain >> 2); > + } else { > + /* subtract 75 % to thread power */ > + smt_gain = smt_gain >> 2; > + } > + } > + } > + > + /* default smt gain is 1178, weight is # of SMT threads */ > + switch (weight) { > + case 1: > + /*divide by 1, do nothing*/ > + break; > + case 2: > + smt_gain = smt_gain >> 1; > + break; > + case 4: > + smt_gain = smt_gain >> 2; > + break; > + default: > + smt_gain /= weight; > + break; > + } > + > + return smt_gain; > +} Appart from that, it looks allright to me. Cheers, Ben.
next prev parent reply other threads:[~2010-01-27 0:55 UTC|newest] Thread overview: 103+ 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:00 ` Joel Schopp 2010-01-20 20:02 ` [PATCH 1/2] sched: Fix the place where group powers are updated Joel Schopp 2010-01-20 20:02 ` Joel Schopp 2010-01-21 13:54 ` [tip:sched/core] " tip-bot for Gautham R Shenoy 2010-01-26 23:28 ` [PATCHv2 1/2] sched: enable ARCH_POWER Joel Schopp 2010-01-26 23:28 ` Joel Schopp 2010-01-28 23:20 ` [PATCHv3 " Joel Schopp 2010-01-28 23:20 ` Joel Schopp 2010-02-05 20:57 ` [PATCHv4 " Joel Schopp 2010-02-05 20:57 ` Joel Schopp 2010-01-20 20:04 ` [PATCH 2/2] powerpc: implement arch_scale_smt_power for Power7 Joel Schopp 2010-01-20 20:04 ` Joel Schopp 2010-01-20 20:48 ` Peter Zijlstra 2010-01-20 20:48 ` Peter Zijlstra 2010-01-20 21:58 ` Michael Neuling 2010-01-20 21:58 ` Michael Neuling 2010-01-20 22:44 ` Joel Schopp 2010-01-20 22:44 ` Joel Schopp 2010-01-21 8:27 ` Peter Zijlstra 2010-01-21 8:27 ` Peter Zijlstra 2010-01-20 21:04 ` Michael Neuling 2010-01-20 21:04 ` Michael Neuling 2010-01-20 22:09 ` Joel Schopp 2010-01-20 22:09 ` Joel Schopp 2010-01-24 3:00 ` Benjamin Herrenschmidt 2010-01-24 3:00 ` Benjamin Herrenschmidt 2010-01-25 17:50 ` Joel Schopp 2010-01-25 17:50 ` Joel Schopp 2010-01-26 4:23 ` Benjamin Herrenschmidt 2010-01-26 4:23 ` Benjamin Herrenschmidt 2010-01-20 21:33 ` Benjamin Herrenschmidt 2010-01-20 21:33 ` Benjamin Herrenschmidt 2010-01-20 22:36 ` Joel Schopp 2010-01-20 22:36 ` Joel Schopp 2010-01-26 23:28 ` [PATCHv2 " Joel Schopp 2010-01-26 23:28 ` Joel Schopp 2010-01-27 0:52 ` Benjamin Herrenschmidt [this message] 2010-01-27 0:52 ` Benjamin Herrenschmidt 2010-01-28 22:39 ` Joel Schopp 2010-01-28 22:39 ` Joel Schopp 2010-01-29 1:23 ` Benjamin Herrenschmidt 2010-01-29 1:23 ` Benjamin Herrenschmidt 2010-01-28 23:20 ` [PATCHv3 " Joel Schopp 2010-01-28 23:20 ` Joel Schopp 2010-01-28 23:24 ` Joel Schopp 2010-01-28 23:24 ` Joel Schopp 2010-01-29 1:23 ` Benjamin Herrenschmidt 2010-01-29 1:23 ` Benjamin Herrenschmidt 2010-01-29 10:13 ` Peter Zijlstra 2010-01-29 10:13 ` Peter Zijlstra 2010-01-29 18:34 ` Joel Schopp 2010-01-29 18:34 ` Joel Schopp 2010-01-29 18:41 ` Joel Schopp 2010-01-29 18:41 ` Joel Schopp 2010-02-05 20:57 ` [PATCHv4 " Joel Schopp 2010-02-05 20:57 ` Joel Schopp 2010-02-14 10:12 ` Peter Zijlstra 2010-02-14 10:12 ` Peter Zijlstra 2010-02-17 22:20 ` Michael Neuling 2010-02-17 22:20 ` Michael Neuling 2010-02-18 13:17 ` Peter Zijlstra 2010-02-18 13:17 ` Peter Zijlstra 2010-02-18 13:19 ` Peter Zijlstra 2010-02-18 13:19 ` Peter Zijlstra 2010-02-18 16:28 ` Joel Schopp 2010-02-18 16:28 ` Joel Schopp 2010-02-18 17:08 ` Peter Zijlstra 2010-02-18 17:08 ` Peter Zijlstra 2010-02-19 6:05 ` Michael Neuling 2010-02-19 6:05 ` Michael Neuling 2010-02-19 10:01 ` Peter Zijlstra 2010-02-19 10:01 ` Peter Zijlstra 2010-02-19 11:01 ` Michael Neuling 2010-02-19 11:01 ` Michael Neuling 2010-02-23 6:08 ` Michael Neuling 2010-02-23 6:08 ` Michael Neuling 2010-02-23 16:24 ` Peter Zijlstra 2010-02-23 16:24 ` Peter Zijlstra 2010-02-23 16:30 ` Peter Zijlstra 2010-02-23 16:30 ` Peter Zijlstra 2010-02-24 6:07 ` Michael Neuling 2010-02-24 6:07 ` Michael Neuling 2010-02-24 11:13 ` Michael Neuling 2010-02-24 11:13 ` Michael Neuling 2010-02-24 11:58 ` Michael Neuling 2010-02-24 11:58 ` Michael Neuling 2010-02-27 10:21 ` Michael Neuling 2010-02-27 10:21 ` Michael Neuling 2010-03-02 14:44 ` Peter Zijlstra 2010-03-02 14:44 ` Peter Zijlstra 2010-03-04 22:28 ` Michael Neuling 2010-03-04 22:28 ` Michael Neuling 2010-01-29 12:25 ` [PATCHv3 " Gabriel Paubert 2010-01-29 12:25 ` Gabriel Paubert 2010-01-29 16:26 ` Joel Schopp 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-26 23:27 ` Joel Schopp 2010-01-28 23:20 ` [PATCHv3 0/2] sched: arch_scale_smt_powers Joel Schopp 2010-01-28 23:20 ` Joel Schopp 2010-02-05 20:57 ` [PATCHv4 " Joel Schopp 2010-02-05 20:57 ` 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=1264553577.3601.144.camel@pasglop \ --to=benh@kernel.crashing.org \ --cc=a.p.zijlstra@chello.nl \ --cc=ego@in.ibm.com \ --cc=jschopp@austin.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.