lkml.org 
[lkml]   [2019]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/3] cpuidle-powernv : forced wakeup for stop states
From
Date
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

\
 
 \ /
  Last update: 2019-07-09 07:35    [W:0.196 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site