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

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Lina Iyer <ilina@codeaurora.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH] cpuidle: record state entry failed statistics
Date: Tue, 22 Sep 2020 17:57:05 +0200	[thread overview]
Message-ID: <CAJZ5v0gWVVPF_S_=1WyXAiB0Gse6PO8VZQf0yAr=in15CM3w7w@mail.gmail.com> (raw)
In-Reply-To: <20200902210116.7360-1-ilina@codeaurora.org>

Sorry for the delay.

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.

>  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"?

>  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
>