Message ID | 20180125050512.GA18744@fergus.ozlabs.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [RFC] powerpc/powernv: Provide a way to force a core into SMT4 mode | expand |
On Thu, 25 Jan 2018 16:05:12 +1100 Paul Mackerras <paulus@ozlabs.org> wrote: > POWER9 processors up to and including "Nimbus" v2.2 have hardware > bugs relating to transactional memory and thread reconfiguration. > One of these bugs has a workaround which is to get the core into > SMT4 state temporarily. This workaround is only needed when > running bare-metal. How often will this be triggered, in practice? If it's infrequent, then would it be better to just do a smp_call_function on siblings and get them all spinning there? I'm looking sadly at the added sync... Thanks, Nick
On Thu, Jan 25, 2018 at 04:05:12PM +1100, Paul Mackerras wrote: > POWER9 processors up to and including "Nimbus" v2.2 have hardware > bugs relating to transactional memory and thread reconfiguration. > One of these bugs has a workaround which is to get the core into > SMT4 state temporarily. This workaround is only needed when > running bare-metal. ..snip.. > > Signed-off-by: Paul Mackerras <paulus@ozlabs.org> > --- > arch/powerpc/include/asm/paca.h | 3 ++ > arch/powerpc/kernel/asm-offsets.c | 1 + > arch/powerpc/kernel/idle_book3s.S | 15 +++++++++ > arch/powerpc/platforms/powernv/idle.c | 62 +++++++++++++++++++++++++++++++++++ > 4 files changed, 81 insertions(+) > ..snip.. > STOP_SPR(STOP_PID, pid); > STOP_SPR(STOP_LDBAR, ldbar); > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S > index 01e1c19..4a7f88c 100644 > --- a/arch/powerpc/kernel/idle_book3s.S > +++ b/arch/powerpc/kernel/idle_book3s.S > @@ -430,10 +430,23 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66); \ > */ > _GLOBAL(power9_idle_stop) > std r3, PACA_REQ_PSSCR(r13) this instruction can go a little later and save a few cycles, in the case it need not have to stop ? > + sync > + lwz r5, PACA_DONT_STOP(r13) > + cmpwi r5, 0 > + bne 1f I mean 'std r3, ...' can move here. > mtspr SPRN_PSSCR,r3 > LOAD_REG_ADDR(r4,power_enter_stop) > b pnv_powersave_common > /* No return */ > +1: > + /* > + * We get here when TM / thread reconfiguration bug workaround > + * code wants to get the CPU into SMT4 mode, and therefore > + * we are being asked not to stop. > + */ > + li r3, 0 > + std r3, PACA_REQ_PSSCR(r13) > + blr /* return 0 for wakeup cause / SRR1 value */ > > /* > * On waking up from stop 0,1,2 with ESL=1 on POWER9 DD1, > @@ -584,6 +597,8 @@ FTR_SECTION_ELSE_NESTED(71) > mfspr r5, SPRN_PSSCR > rldicl r5,r5,4,60 > ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_POWER9_DD1, 71) > + li r0, 0 /* clear requested_psscr to say we're awake */ > + std r0, PACA_REQ_PSSCR(r13) > cmpd cr4,r5,r4 > bge cr4,pnv_wakeup_tb_loss /* returns to caller */ > > diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c > index 443d5ca..72d5a85 100644 > --- a/arch/powerpc/platforms/powernv/idle.c > +++ b/arch/powerpc/platforms/powernv/idle.c > @@ -24,6 +24,7 @@ > #include <asm/code-patching.h> > #include <asm/smp.h> > #include <asm/runlatch.h> > +#include <asm/dbell.h> > > #include "powernv.h" > #include "subcore.h" > @@ -387,6 +388,67 @@ void power9_idle(void) > power9_idle_type(pnv_default_stop_val, pnv_default_stop_mask); > } > > +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE > +/* > + * This is used in working around bugs in thread reconfiguration > + * on POWER9 (at least up to Nimbus DD2.2) relating to transactional > + * memory and the way that XER[SO] is checkpointed. > + * This function forces the core into SMT4 in order by asking > + * all other threads not to stop, and sending a message to any > + * that are in a stop state. > + * Must be called with preemption disabled. > + */ > +void pnv_power9_force_smt4(void) > +{ > + int cpu, cpu0, thr; > + struct paca_struct *tpaca; > + int awake_threads = 1; /* this thread is awake */ > + int poke_threads = 0; > + > + cpu = smp_processor_id(); > + cpu0 = cpu & ~(threads_per_core - 1); > + tpaca = &paca[cpu0]; > + for (thr = 0; thr < threads_per_core; ++thr) { > + if (cpu != cpu0 + thr) > + atomic_inc(&tpaca[thr].dont_stop); > + } > + /* order setting dont_stop vs testing requested_psscr */ > + mb(); > + for (thr = 0; thr < threads_per_core; ++thr) { > + if (!tpaca[thr].requested_psscr) > + ++awake_threads; > + else > + poke_threads |= (1 << thr); ppc_msgsnd(...) can be called here in the else part? > + } > + > + /* If at least 3 threads are awake, the core is in SMT4 already */ small nitpick -- this comment mentions SMT4 and 3 threads. But the code is generically applicable to SMTn and (n-1) threads. > + if (awake_threads < threads_per_core - 1) { > + /* We have to wake some threads; we'll use msgsnd */ > + for (thr = 0; thr < threads_per_core; ++thr) { > + if (poke_threads & (1 << thr)) > + ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, > + tpaca[thr].hw_cpu_id); > + } and this loop can be deleted, which inturn can leads to further optimizations. > + /* now spin until at least 3 threads are awake */ > + do { > + for (thr = 0; thr < threads_per_core; ++thr) { > + if ((poke_threads & (1 << thr)) && > + !tpaca[thr].requested_psscr) { > + ++awake_threads; > + poke_threads &= ~(1 << thr); > + } > + } > + } while (awake_threads < threads_per_core - 1); > + } > + /* clear all the dont_stop flags */ > + for (thr = 0; thr < threads_per_core; ++thr) { > + if (cpu != cpu0 + thr) > + atomic_dec(&tpaca[thr].dont_stop); > + } > +} > +EXPORT_SYMBOL_GPL(pnv_power9_force_smt4); > +#endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */ > + > #ifdef CONFIG_HOTPLUG_CPU > static void pnv_program_cpu_hotplug_lpcr(unsigned int cpu, u64 lpcr_val) > { > -- > 2.7.4
On Fri, Jan 26, 2018 at 05:06:10PM -0800, Ram Pai wrote: > On Thu, Jan 25, 2018 at 04:05:12PM +1100, Paul Mackerras wrote: > > POWER9 processors up to and including "Nimbus" v2.2 have hardware > > bugs relating to transactional memory and thread reconfiguration. > > One of these bugs has a workaround which is to get the core into > > SMT4 state temporarily. This workaround is only needed when > > running bare-metal. > > ..snip.. > > */ > > _GLOBAL(power9_idle_stop) > > std r3, PACA_REQ_PSSCR(r13) > > this instruction can go a little later and save a few cycles, in the > case it need not have to stop ? > > > + sync > > + lwz r5, PACA_DONT_STOP(r13) > > + cmpwi r5, 0 > > + bne 1f > > I mean 'std r3, ...' can move here. That would introduce a race condition, where this thread would miss seeing the other thread's store to paca->dont_stop, and the other thread would miss seeing this thread's store to paca->requested_psscr. > > + /* order setting dont_stop vs testing requested_psscr */ > > + mb(); > > + for (thr = 0; thr < threads_per_core; ++thr) { > > + if (!tpaca[thr].requested_psscr) > > + ++awake_threads; > > + else > > + poke_threads |= (1 << thr); > > ppc_msgsnd(...) can be called here in the else part? It could, but I wanted to avoid disturbing the other threads with the msgsnd if it was not necessary. Hence the second loop to do the msgsnds once we have determined that we really need to do them. > > > + } > > + > > + /* If at least 3 threads are awake, the core is in SMT4 already */ > > small nitpick -- this comment mentions SMT4 and 3 threads. But the code > is generically applicable to SMTn and (n-1) threads. Sure - it's easier to understand a concrete example than something more general, that's why the comment is about the specific use case not the general capability of the code. > > + if (awake_threads < threads_per_core - 1) { > > > > + /* We have to wake some threads; we'll use msgsnd */ > > + for (thr = 0; thr < threads_per_core; ++thr) { > > + if (poke_threads & (1 << thr)) > > + ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, > > + tpaca[thr].hw_cpu_id); > > + } > > and this loop can be deleted, which inturn can leads to further optimizations. ... at the cost of other threads taking doorbell interrupts unnecessarily. I thought it better to put more burden on the thread needing this synchronization and less on the other threads (on average). Paul.
On Sat, Jan 27, 2018 at 10:27:35AM +1000, Nicholas Piggin wrote: > On Thu, 25 Jan 2018 16:05:12 +1100 > Paul Mackerras <paulus@ozlabs.org> wrote: > > > POWER9 processors up to and including "Nimbus" v2.2 have hardware > > bugs relating to transactional memory and thread reconfiguration. > > One of these bugs has a workaround which is to get the core into > > SMT4 state temporarily. This workaround is only needed when > > running bare-metal. > > How often will this be triggered, in practice? If it's infrequent, > then would it be better to just do a smp_call_function on siblings > and get them all spinning there? I'm looking sadly at the added > sync... We'll need to do this every time we exit a guest vcpu and the CPU is in "fake suspend" state, which will be the next exit after entering the vcpu when its MSR[TS] = 0b01 (suspend state). If the vcpu does a tresume or treclaim in fake suspend state, that causes a softpatch interrupt; the CPU doesn't get out of fake suspend state because of any guest instruction, only via hypervisor action. So it could be very rare or it could be quite frequent, depending on how much usage the guest makes of TM and how long it spends in suspend state. The smp_call_function on siblings wouldn't work in the case where some threads are off-line, since it only works on online CPUs. Also we would need to spin in the function being called on the other CPUs (otherwise you could get the situation where they wake up serially and you never have 3 or 4 threads simultaneously active), which would make me worry about deadlocks in the case where multiple threads are concurrently trying to get the core into SMT4 mode. If you can think of a way to eliminate the sync without introducing a race, I'm all ears. I haven't been able to. Paul.
On Sat, 27 Jan 2018 13:45:46 +1100 Paul Mackerras <paulus@ozlabs.org> wrote: > On Sat, Jan 27, 2018 at 10:27:35AM +1000, Nicholas Piggin wrote: > > On Thu, 25 Jan 2018 16:05:12 +1100 > > Paul Mackerras <paulus@ozlabs.org> wrote: > > > > > POWER9 processors up to and including "Nimbus" v2.2 have hardware > > > bugs relating to transactional memory and thread reconfiguration. > > > One of these bugs has a workaround which is to get the core into > > > SMT4 state temporarily. This workaround is only needed when > > > running bare-metal. > > > > How often will this be triggered, in practice? If it's infrequent, > > then would it be better to just do a smp_call_function on siblings > > and get them all spinning there? I'm looking sadly at the added > > sync... > > We'll need to do this every time we exit a guest vcpu and the CPU is > in "fake suspend" state, which will be the next exit after entering > the vcpu when its MSR[TS] = 0b01 (suspend state). If the vcpu does a > tresume or treclaim in fake suspend state, that causes a softpatch > interrupt; the CPU doesn't get out of fake suspend state because of > any guest instruction, only via hypervisor action. > > So it could be very rare or it could be quite frequent, depending on > how much usage the guest makes of TM and how long it spends in suspend > state. > > The smp_call_function on siblings wouldn't work in the case where some > threads are off-line, since it only works on online CPUs. Also we > would need to spin in the function being called on the other CPUs > (otherwise you could get the situation where they wake up serially and > you never have 3 or 4 threads simultaneously active), which would make > me worry about deadlocks in the case where multiple threads are > concurrently trying to get the core into SMT4 mode. > > If you can think of a way to eliminate the sync without introducing a > race, I'm all ears. I haven't been able to. Okay thanks for the details, yes it would have to be more complex than a NULL function, I didn't realize offline CPUs would have to be involved. I'll have a think about it. A sync is about 1% of the stop/wake overhead, e.g., measured on P9 here http://patchwork.ozlabs.org/patch/839017/ So it's not a showstopper. The approach seems like it should work AFAIKS. Thanks, Nick
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h index 23ac7fc..71b5c34 100644 --- a/arch/powerpc/include/asm/paca.h +++ b/arch/powerpc/include/asm/paca.h @@ -32,6 +32,7 @@ #include <asm/accounting.h> #include <asm/hmi.h> #include <asm/cpuidle.h> +#include <asm/atomic.h> register struct paca_struct *local_paca asm("r13"); @@ -177,6 +178,8 @@ struct paca_struct { u8 thread_mask; /* Mask to denote subcore sibling threads */ u8 subcore_sibling_mask; + /* Flag to request this thread not to stop */ + atomic_t dont_stop; /* * Pointer to an array which contains pointer * to the sibling threads' paca. diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c index ff6ce2f..91cb8df 100644 --- a/arch/powerpc/kernel/asm-offsets.c +++ b/arch/powerpc/kernel/asm-offsets.c @@ -758,6 +758,7 @@ int main(void) OFFSET(PACA_SUBCORE_SIBLING_MASK, paca_struct, subcore_sibling_mask); OFFSET(PACA_SIBLING_PACA_PTRS, paca_struct, thread_sibling_pacas); OFFSET(PACA_REQ_PSSCR, paca_struct, requested_psscr); + OFFSET(PACA_DONT_STOP, paca_struct, dont_stop); #define STOP_SPR(x, f) OFFSET(x, paca_struct, stop_sprs.f) STOP_SPR(STOP_PID, pid); STOP_SPR(STOP_LDBAR, ldbar); diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S index 01e1c19..4a7f88c 100644 --- a/arch/powerpc/kernel/idle_book3s.S +++ b/arch/powerpc/kernel/idle_book3s.S @@ -430,10 +430,23 @@ ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66); \ */ _GLOBAL(power9_idle_stop) std r3, PACA_REQ_PSSCR(r13) + sync + lwz r5, PACA_DONT_STOP(r13) + cmpwi r5, 0 + bne 1f mtspr SPRN_PSSCR,r3 LOAD_REG_ADDR(r4,power_enter_stop) b pnv_powersave_common /* No return */ +1: + /* + * We get here when TM / thread reconfiguration bug workaround + * code wants to get the CPU into SMT4 mode, and therefore + * we are being asked not to stop. + */ + li r3, 0 + std r3, PACA_REQ_PSSCR(r13) + blr /* return 0 for wakeup cause / SRR1 value */ /* * On waking up from stop 0,1,2 with ESL=1 on POWER9 DD1, @@ -584,6 +597,8 @@ FTR_SECTION_ELSE_NESTED(71) mfspr r5, SPRN_PSSCR rldicl r5,r5,4,60 ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_POWER9_DD1, 71) + li r0, 0 /* clear requested_psscr to say we're awake */ + std r0, PACA_REQ_PSSCR(r13) cmpd cr4,r5,r4 bge cr4,pnv_wakeup_tb_loss /* returns to caller */ diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c index 443d5ca..72d5a85 100644 --- a/arch/powerpc/platforms/powernv/idle.c +++ b/arch/powerpc/platforms/powernv/idle.c @@ -24,6 +24,7 @@ #include <asm/code-patching.h> #include <asm/smp.h> #include <asm/runlatch.h> +#include <asm/dbell.h> #include "powernv.h" #include "subcore.h" @@ -387,6 +388,67 @@ void power9_idle(void) power9_idle_type(pnv_default_stop_val, pnv_default_stop_mask); } +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE +/* + * This is used in working around bugs in thread reconfiguration + * on POWER9 (at least up to Nimbus DD2.2) relating to transactional + * memory and the way that XER[SO] is checkpointed. + * This function forces the core into SMT4 in order by asking + * all other threads not to stop, and sending a message to any + * that are in a stop state. + * Must be called with preemption disabled. + */ +void pnv_power9_force_smt4(void) +{ + int cpu, cpu0, thr; + struct paca_struct *tpaca; + int awake_threads = 1; /* this thread is awake */ + int poke_threads = 0; + + cpu = smp_processor_id(); + cpu0 = cpu & ~(threads_per_core - 1); + tpaca = &paca[cpu0]; + for (thr = 0; thr < threads_per_core; ++thr) { + if (cpu != cpu0 + thr) + atomic_inc(&tpaca[thr].dont_stop); + } + /* order setting dont_stop vs testing requested_psscr */ + mb(); + for (thr = 0; thr < threads_per_core; ++thr) { + if (!tpaca[thr].requested_psscr) + ++awake_threads; + else + poke_threads |= (1 << thr); + } + + /* If at least 3 threads are awake, the core is in SMT4 already */ + if (awake_threads < threads_per_core - 1) { + /* We have to wake some threads; we'll use msgsnd */ + for (thr = 0; thr < threads_per_core; ++thr) { + if (poke_threads & (1 << thr)) + ppc_msgsnd(PPC_DBELL_MSGTYPE, 0, + tpaca[thr].hw_cpu_id); + } + /* now spin until at least 3 threads are awake */ + do { + for (thr = 0; thr < threads_per_core; ++thr) { + if ((poke_threads & (1 << thr)) && + !tpaca[thr].requested_psscr) { + ++awake_threads; + poke_threads &= ~(1 << thr); + } + } + } while (awake_threads < threads_per_core - 1); + } + /* clear all the dont_stop flags */ + for (thr = 0; thr < threads_per_core; ++thr) { + if (cpu != cpu0 + thr) + atomic_dec(&tpaca[thr].dont_stop); + } +} +EXPORT_SYMBOL_GPL(pnv_power9_force_smt4); +#endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */ + #ifdef CONFIG_HOTPLUG_CPU static void pnv_program_cpu_hotplug_lpcr(unsigned int cpu, u64 lpcr_val) {
POWER9 processors up to and including "Nimbus" v2.2 have hardware bugs relating to transactional memory and thread reconfiguration. One of these bugs has a workaround which is to get the core into SMT4 state temporarily. This workaround is only needed when running bare-metal. This patch provides a function which gets the core into SMT4 mode by preventing threads from going to a stop state, and waking up those which are already in a stop state. Once at least 3 threads are not in a stop state, the core will be in SMT4 and we can continue. To do this, we add a "dont_stop" flag to the paca to tell the thread not to go into a stop state. If this flag is set, power9_idle_stop() just returns immediately with a return value of 0. The pnv_power9_force_smt4() function does the following: 1. Set the dont_stop flag for each thread in the core, except ourselves (in fact we use an atomic_inc() in case more than one thread is calling this function concurrently). 2. See how many threads are awake, indicated by their requested_psscr field in the paca being 0. If this is at least 3, skip to step 5. 3. Send a doorbell interrupt to each thread that was seen as being in a stop state in step 2. 4. Until at least 3 threads are awake, scan the threads to which we sent a doorbell interrupt and check if they are awake now. 5. Clear (actually atomic_dec()) the dont_stop flag for each thread in the core, except for ourselves. This relies on the following properties: - Once dont_stop is non-zero, requested_psccr can't go from zero to non-zero, except transiently (and without the thread doing stop). - requested_psscr being zero guarantees that the thread isn't in a state-losing stop state where thread reconfiguration could occur. - Doing stop with a PSSCR value of 0 won't be a state-losing stop and thus won't allow thread reconfiguration. This does add a sync to power9_idle_stop(), which is necessary to provide the correct ordering between setting requested_psscr and checking dont_stop. The overhead of the sync should be unnoticeable compared to the latency of going into and out of a stop state. Signed-off-by: Paul Mackerras <paulus@ozlabs.org> --- arch/powerpc/include/asm/paca.h | 3 ++ arch/powerpc/kernel/asm-offsets.c | 1 + arch/powerpc/kernel/idle_book3s.S | 15 +++++++++ arch/powerpc/platforms/powernv/idle.c | 62 +++++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+)