Re: [PATCH] cpuidle: record state entry failed statistics

[Lina Iyer <ilina@codeaurora.org>]

On Tue, Sep 22 2020 at 09:57 -0600, Rafael J. Wysocki wrote:
>Sorry for the delay.
>
Thanks for the review.

>On Wed, Sep 2, 2020 at 11:01 PM Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> When CPUs fail to enter the chosen idle state it's mostly because of a
>> pending interrupt. Let's record that and show along with other
>> statistics for the idle state. This could prove useful in understanding
>> behavior of the governor and the system during usecases that involve
>> multiple CPUs.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  drivers/cpuidle/cpuidle.c | 1 +
>>  drivers/cpuidle/sysfs.c   | 3 +++
>>  include/linux/cpuidle.h   | 1 +
>
>The documentation needs to be updated too to cover the new state attribute.
>
Sure, will add that in the next spin.
>>  3 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index 04becd70cc41..8dbf71f6138d 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -302,6 +302,7 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>>                 }
>>         } else {
>>                 dev->last_residency_ns = 0;
>> +               dev->states_usage[index].failed++;
>>         }
>>
>>         return entered_state;
>> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
>> index 091d1caceb41..f166687b3bcd 100644
>> --- a/drivers/cpuidle/sysfs.c
>> +++ b/drivers/cpuidle/sysfs.c
>> @@ -256,6 +256,7 @@ define_show_state_time_function(exit_latency)
>>  define_show_state_time_function(target_residency)
>>  define_show_state_function(power_usage)
>>  define_show_state_ull_function(usage)
>> +define_show_state_ull_function(failed)
>
>And what about calling it "rejected" instead of "failed"?
>
Works for me.

Thanks,
Lina

>>  define_show_state_str_function(name)
>>  define_show_state_str_function(desc)
>>  define_show_state_ull_function(above)
>> @@ -312,6 +313,7 @@ define_one_state_ro(latency, show_state_exit_latency);
>>  define_one_state_ro(residency, show_state_target_residency);
>>  define_one_state_ro(power, show_state_power_usage);
>>  define_one_state_ro(usage, show_state_usage);
>> +define_one_state_ro(failed, show_state_failed);
>>  define_one_state_ro(time, show_state_time);
>>  define_one_state_rw(disable, show_state_disable, store_state_disable);
>>  define_one_state_ro(above, show_state_above);
>> @@ -325,6 +327,7 @@ static struct attribute *cpuidle_state_default_attrs[] = {
>>         &attr_residency.attr,
>>         &attr_power.attr,
>>         &attr_usage.attr,
>> +       &attr_failed.attr,
>>         &attr_time.attr,
>>         &attr_disable.attr,
>>         &attr_above.attr,
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 75895e6363b8..911b99273eba 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -38,6 +38,7 @@ struct cpuidle_state_usage {
>>         u64                     time_ns;
>>         unsigned long long      above; /* Number of times it's been too deep */
>>         unsigned long long      below; /* Number of times it's been too shallow */
>> +       unsigned long long      failed; /* Number of times it failed to enter */
>>  #ifdef CONFIG_SUSPEND
>>         unsigned long long      s2idle_usage;
>>         unsigned long long      s2idle_time; /* in US */
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>