Re: [linux-pm] [PATCH v4] cpuidle: Add a sysfs entry to disablespecific C state for debug purpose.

From: Yanmin Zhang
Date: Thu Mar 15 2012 - 20:23:46 EST

Next message: Tony Breeds: "[PATCH] Replace IS_ENABLED(CONFIG_FCOE) in linux/netdevice.h"
Previous message: Arun Sharma: "[PATCH] [RFC] perf: new semantics for callgraph sym filtering"
In reply to: ShuoX Liu: "[linux-pm] [PATCH v4] cpuidle: Add a sysfs entry to disable specificC state for debug purpose."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 2012-03-14 at 11:24 +0800, ShuoX Liu wrote:
> From: ShuoX Liu <shuox.liu@xxxxxxxxx>
>
> Some C states of new CPU might be not good. One reason is BIOS might configure
> them incorrectly. To help developers root cause it quickly, the patch adds a
> new sysfs entry, so developers could disable specific C state manually.
>
> In addition, C state might have much impact on performance tuning, as it takes
> much time to enter/exit C states, which might delay interrupt processing. With
> the new debug option, developers could check if a deep C state could impact
> performance and how much impact it could cause.
>
> Also add this option in Documentation/cpuidle/sysfs.txt.
Andrew,

Would you like to merge the new patch into your testing tree? Shuo revised the old
patches based on all comments from community.

Deepthi confirms the patch is useful.

Thanks,
Yanmin

>
> Signed-off-by: ShuoX Liu <shuox.liu@xxxxxxxxx>
> Reviewed-by: Yanmin Zhang <yanmin_zhang@xxxxxxxxx>
> Reviewed-and-Tested-by: Deepthi Dharwar <deepthi@xxxxxxxxxxxxxxxxxx>
> ---
> Thanks for all comments in previous mails. Here is the new patch.
>
> ---
> Documentation/cpuidle/sysfs.txt | 5 +++++
> drivers/cpuidle/cpuidle.c | 1 +
> drivers/cpuidle/governors/menu.c | 5 ++++-
> drivers/cpuidle/sysfs.c | 37 +++++++++++++++++++++++++++++++++++++
> include/linux/cpuidle.h | 1 +
> 5 files changed, 48 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/cpuidle/sysfs.txt b/Documentation/cpuidle/sysfs.txt
> index 50d7b16..9d28a34 100644
> --- a/Documentation/cpuidle/sysfs.txt
> +++ b/Documentation/cpuidle/sysfs.txt
> @@ -36,6 +36,7 @@ drwxr-xr-x 2 root root 0 Feb 8 10:42 state3
> /sys/devices/system/cpu/cpu0/cpuidle/state0:
> total 0
> -r--r--r-- 1 root root 4096 Feb 8 10:42 desc
> +-rw-r--r-- 1 root root 4096 Feb 8 10:42 disable
> -r--r--r-- 1 root root 4096 Feb 8 10:42 latency
> -r--r--r-- 1 root root 4096 Feb 8 10:42 name
> -r--r--r-- 1 root root 4096 Feb 8 10:42 power
> @@ -45,6 +46,7 @@ total 0
> /sys/devices/system/cpu/cpu0/cpuidle/state1:
> total 0
> -r--r--r-- 1 root root 4096 Feb 8 10:42 desc
> +-rw-r--r-- 1 root root 4096 Feb 8 10:42 disable
> -r--r--r-- 1 root root 4096 Feb 8 10:42 latency
> -r--r--r-- 1 root root 4096 Feb 8 10:42 name
> -r--r--r-- 1 root root 4096 Feb 8 10:42 power
> @@ -54,6 +56,7 @@ total 0
> /sys/devices/system/cpu/cpu0/cpuidle/state2:
> total 0
> -r--r--r-- 1 root root 4096 Feb 8 10:42 desc
> +-rw-r--r-- 1 root root 4096 Feb 8 10:42 disable
> -r--r--r-- 1 root root 4096 Feb 8 10:42 latency
> -r--r--r-- 1 root root 4096 Feb 8 10:42 name
> -r--r--r-- 1 root root 4096 Feb 8 10:42 power
> @@ -63,6 +66,7 @@ total 0
> /sys/devices/system/cpu/cpu0/cpuidle/state3:
> total 0
> -r--r--r-- 1 root root 4096 Feb 8 10:42 desc
> +-rw-r--r-- 1 root root 4096 Feb 8 10:42 disable
> -r--r--r-- 1 root root 4096 Feb 8 10:42 latency
> -r--r--r-- 1 root root 4096 Feb 8 10:42 name
> -r--r--r-- 1 root root 4096 Feb 8 10:42 power
> @@ -72,6 +76,7 @@ total 0
>
>
> * desc : Small description about the idle state (string)
> +* disable : Option to disable this idle state (bool)
> * latency : Latency to exit out of this idle state (in microseconds)
> * name : Name of the idle state (string)
> * power : Power consumed while in this idle state (in milliwatts)
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 59f4261..7d66d3e 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -197,6 +197,7 @@ static void poll_idle_init(struct cpuidle_driver *drv)
> state->power_usage = -1;
> state->flags = 0;
> state->enter = poll_idle;
> + state->disable = 0;
> }
> #else
> static void poll_idle_init(struct cpuidle_driver *drv) {}
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index ad09526..5c17ca1 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -280,7 +280,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> * We want to default to C1 (hlt), not to busy polling
> * unless the timer is happening really really soon.
> */
> - if (data->expected_us > 5)
> + if (data->expected_us > 5 &&
> + drv->states[CPUIDLE_DRIVER_STATE_START].disable == 0)
> data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
>
> /*
> @@ -290,6 +291,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
> struct cpuidle_state *s = &drv->states[i];
>
> + if (s->disable)
> + continue;
> if (s->target_residency > data->predicted_us)
> continue;
> if (s->exit_latency > latency_req)
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index 3fe41fe..f0fe0f5 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -11,6 +11,7 @@
> #include <linux/sysfs.h>
> #include <linux/slab.h>
> #include <linux/cpu.h>
> +#include <linux/capability.h>
>
> #include "cpuidle.h"
>
> @@ -222,6 +223,9 @@ struct cpuidle_state_attr {
> #define define_one_state_ro(_name, show) \
> static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0444, show, NULL)
>
> +#define define_one_state_rw(_name, show, store) \
> +static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644, show, store)
> +
> #define define_show_state_function(_name) \
> static ssize_t show_state_##_name(struct cpuidle_state *state, \
> struct cpuidle_state_usage *state_usage, char *buf) \
> @@ -229,6 +233,21 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \
> return sprintf(buf, "%u\n", state->_name);\
> }
>
> +#define define_store_state_function(_name) \
> +static ssize_t store_state_##_name(struct cpuidle_state *state, \
> + const char *buf, size_t size) \
> +{ \
> + long value; \
> + if (!capable(CAP_SYS_ADMIN)) \
> + return -EPERM; \
> + kstrtol(buf, 0, &value); \
> + if (value) \
> + state->disable = 1; \
> + else \
> + state->disable = 0; \
> + return size; \
> +}
> +
> #define define_show_state_ull_function(_name) \
> static ssize_t show_state_##_name(struct cpuidle_state *state, \
> struct cpuidle_state_usage *state_usage, char *buf) \
> @@ -251,6 +270,8 @@ define_show_state_ull_function(usage)
> define_show_state_ull_function(time)
> define_show_state_str_function(name)
> define_show_state_str_function(desc)
> +define_show_state_function(disable)
> +define_store_state_function(disable)
>
> define_one_state_ro(name, show_state_name);
> define_one_state_ro(desc, show_state_desc);
> @@ -258,6 +279,7 @@ define_one_state_ro(latency, show_state_exit_latency);
> define_one_state_ro(power, show_state_power_usage);
> define_one_state_ro(usage, show_state_usage);
> define_one_state_ro(time, show_state_time);
> +define_one_state_rw(disable, show_state_disable, store_state_disable);
>
> static struct attribute *cpuidle_state_default_attrs[] = {
> &attr_name.attr,
> @@ -266,6 +288,7 @@ static struct attribute *cpuidle_state_default_attrs[] = {
> &attr_power.attr,
> &attr_usage.attr,
> &attr_time.attr,
> + &attr_disable.attr,
> NULL
> };
>
> @@ -287,8 +310,22 @@ static ssize_t cpuidle_state_show(struct kobject * kobj,
> return ret;
> }
>
> +static ssize_t cpuidle_state_store(struct kobject *kobj,
> + struct attribute *attr, const char *buf, size_t size)
> +{
> + int ret = -EIO;
> + struct cpuidle_state *state = kobj_to_state(kobj);
> + struct cpuidle_state_attr *cattr = attr_to_stateattr(attr);
> +
> + if (cattr->store)
> + ret = cattr->store(state, buf, size);
> +
> + return ret;
> +}
> +
> static const struct sysfs_ops cpuidle_state_sysfs_ops = {
> .show = cpuidle_state_show,
> + .store = cpuidle_state_store,
> };
>
> static void cpuidle_state_sysfs_release(struct kobject *kobj)
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 712abcc..eb150a5 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -45,6 +45,7 @@ struct cpuidle_state {
> unsigned int exit_latency; /* in US */
> unsigned int power_usage; /* in mW */
> unsigned int target_residency; /* in US */
> + unsigned int disable;
>
> int (*enter) (struct cpuidle_device *dev,
> struct cpuidle_driver *drv,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Tony Breeds: "[PATCH] Replace IS_ENABLED(CONFIG_FCOE) in linux/netdevice.h"
Previous message: Arun Sharma: "[PATCH] [RFC] perf: new semantics for callgraph sym filtering"
In reply to: ShuoX Liu: "[linux-pm] [PATCH v4] cpuidle: Add a sysfs entry to disable specificC state for debug purpose."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]