Messages in this thread | | | Subject | Re: [PATCH v3 1/3] cpuidle-powernv : forced wakeup for stop states | From | Abhishek <> | Date | Tue, 9 Jul 2019 11:02:50 +0530 |
| |
Hi Nick,
Will post next version with the changes you have suggested. There is a comment below.
On 07/07/2019 03:43 PM, Nicholas Piggin wrote: > Abhishek Goel's on July 4, 2019 7:18 pm: >> Currently, the cpuidle governors determine what idle state a idling CPU >> should enter into based on heuristics that depend on the idle history on >> that CPU. Given that no predictive heuristic is perfect, there are cases >> where the governor predicts a shallow idle state, hoping that the CPU will >> be busy soon. However, if no new workload is scheduled on that CPU in the >> near future, the CPU may end up in the shallow state. >> >> This is problematic, when the predicted state in the aforementioned >> scenario is a shallow stop state on a tickless system. As we might get >> stuck into shallow states for hours, in absence of ticks or interrupts. >> >> To address this, We forcefully wakeup the cpu by setting the >> decrementer. The decrementer is set to a value that corresponds with the >> residency of the next available state. Thus firing up a timer that will >> forcefully wakeup the cpu. Few such iterations will essentially train the >> governor to select a deeper state for that cpu, as the timer here >> corresponds to the next available cpuidle state residency. Thus, cpu will >> eventually end up in the deepest possible state. >> >> Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com> >> --- >> >> Auto-promotion >> v1 : started as auto promotion logic for cpuidle states in generic >> driver >> v2 : Removed timeout_needed and rebased the code to upstream kernel >> Forced-wakeup >> v1 : New patch with name of forced wakeup started >> v2 : Extending the forced wakeup logic for all states. Setting the >> decrementer instead of queuing up a hrtimer to implement the logic. >> v3 : Cleanly handle setting/resetting of decrementer so as to not break >> irq work >> >> arch/powerpc/include/asm/time.h | 2 ++ >> arch/powerpc/kernel/time.c | 40 +++++++++++++++++++++++++++++++ >> drivers/cpuidle/cpuidle-powernv.c | 32 +++++++++++++++++++++++++ >> 3 files changed, 74 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h >> index 54f4ec1f9..a3bd4f3c0 100644 >> --- a/arch/powerpc/include/asm/time.h >> +++ b/arch/powerpc/include/asm/time.h >> @@ -188,6 +188,8 @@ static inline unsigned long tb_ticks_since(unsigned long tstamp) >> extern u64 mulhdu(u64, u64); >> #endif >> >> +extern int set_dec_before_idle(u64 timeout); >> +extern void reset_dec_after_idle(void); >> extern void div128_by_32(u64 dividend_high, u64 dividend_low, >> unsigned divisor, struct div_result *dr); >> >> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c >> index 694522308..814de3469 100644 >> --- a/arch/powerpc/kernel/time.c >> +++ b/arch/powerpc/kernel/time.c >> @@ -576,6 +576,46 @@ void arch_irq_work_raise(void) >> >> #endif /* CONFIG_IRQ_WORK */ >> >> +/* >> + * Returns 1 if we have reprogrammed the decrementer for idle. >> + * Returns 0 if the decrementer is unchanged. >> + */ >> +int set_dec_before_idle(u64 timeout) >> +{ >> + u64 *next_tb = this_cpu_ptr(&decrementers_next_tb); >> + u64 now = get_tb_or_rtc(); >> + >> + /* >> + * Ensure that the timeout is at least one microsecond >> + * before the current decrement value. Else, we will >> + * unnecesarily wakeup again within a microsecond. >> + */ >> + if (now + timeout + 512 > *next_tb) > I would pass this 512 in as a parameter and put the comment in the > idle code. Timer code does not know/care. > > Maybe return bool and call it try_set_dec_before_idle. >> + return 0; >> + >> + set_dec(timeout); > This needs to have > > if (test_irq_work_pending()) > set_dec(1); > > here AFAIKS > >> + >> + return 1; >> +} >> + >> +void reset_dec_after_idle(void) >> +{ >> + u64 now; >> + u64 *next_tb; >> + >> + if (test_irq_work_pending()) >> + return; >> + >> + now = get_tb_or_rtc(); >> + next_tb = this_cpu_ptr(&decrementers_next_tb); >> + if (now >= *next_tb) >> + return; > Are you sure it's okay to escape early in this case?
Yeah, It looks safe. In power9_idle_type, we call irq_set_pending_from_srr1 which sets the irq_happened. If reason is IRQ_DEC, in __check_irq_replay, decrementer_check_overflow will be called which will set dec to a positive valid value. Also, we typically disable MSR EE before entering stop. And if a decrementer wakes us up, before we enable EE, check for pending interrupt will be done. And we finally reset dec to a positive value before we set EE=1. > Thanks, > Nick >
Thanks, Abhishek
| |