Subject: [PATCH] cpuidle: Add a sysfs entry to disable specific C state for debug purpose.

From: ShuoX Liu <[email protected]>

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.

Signed-off-by ShuoX Liu <[email protected]>

---
 drivers/cpuidle/cpuidle.c        |    1 +
 drivers/cpuidle/governors/menu.c |    5 ++++-
 drivers/cpuidle/sysfs.c          |   34 ++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h          |    1 +
 4 files changed, 40 insertions(+), 1 deletions(-)

drivers/cpuidle/cpuidle.c +1 -0

index 59f4261..7d66d3e 100644
@@ -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) {}

drivers/cpuidle/governors/menu.c +4 -1

index ad09526..5c17ca1 100644
@@ -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)

drivers/cpuidle/sysfs.c +34 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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 +232,19 @@ 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) \
+{ \
+	int value; \
+	sscanf(buf, "%d", &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 +267,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 +276,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 +285,7 @@ static struct attribute *cpuidle_state_default_attrs[] = {
 	&attr_power.attr,
 	&attr_usage.attr,
 	&attr_time.attr,
+	&attr_disable.attr,
 	NULL
 };
 

@@ -287,8 +307,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)

include/linux/cpuidle.h +1 -0

index 712abcc..eb150a5 100644
@@ -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,

Quoted 11 lines · ShuoX

From: ShuoX Liu <[email protected]>

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.

Quoted 1 line

Signed-off-by ShuoX Liu <[email protected]>

Andrew,

Would you like to merge the patch into your testing tree?

It's useful to help developers debug some issues.

Quoted 12 lines · On Fri, 02 Mar 2012 08:17:33 +0800 Yanmin Zhang <[email protected]>…

Quoted 11 lines · ShuoX

From: ShuoX Liu <[email protected]>

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.

Quoted 4 lines

Quoted 1 line

Signed-off-by ShuoX Liu <[email protected]>

Andrew,

Would you like to merge the patch into your testing tree?

I can do that, but it's hardly my area and I can't comment on the desirability of the patch and Len is having quiet time. Perhaps the x86 guys can take a look please>

Quoted 1 line

It's useful to help developers debug some issues.

It would help developers more if it was documented a bit. As it stands, they'd be lucky if they even knew it existed.

Looky:

akpm:/usr/src/linux-3.3-rc5> grep -ril cpuidle Documentation Documentation/trace/events-power.txt Documentation/ABI/testing/sysfs-devices-system-cpu Documentation/kernel-parameters.txt Documentation/00-INDEX Documentation/cpuidle/core.txt Documentation/cpuidle/sysfs.txt Documentation/cpuidle/driver.txt Documentation/cpuidle/governor.txt

Quoted 1 line

It's useful to help developers debug some issues.

It would help developers more if it was documented a bit. As it stands, they'd be lucky if they even knew it existed.

Looky:

akpm:/usr/src/linux-3.3-rc5> grep -ril cpuidle Documentation Documentation/trace/events-power.txt Documentation/ABI/testing/sysfs-devices-system-cpu Documentation/kernel-parameters.txt Documentation/00-INDEX Documentation/cpuidle/core.txt Documentation/cpuidle/sysfs.txt Documentation/cpuidle/driver.txt Documentation/cpuidle/governor.txt

Quoted 7 lines

Quoted 1 line

It's useful to help developers debug some issues.

It would help developers more if it was documented a bit. As it stands, they'd be lucky if they even knew it existed.

Looky:

Quoted 9 lines

akpm:/usr/src/linux-3.3-rc5> grep -ril cpuidle Documentation Documentation/trace/events-power.txt Documentation/ABI/testing/sysfs-devices-system-cpu Documentation/kernel-parameters.txt Documentation/00-INDEX Documentation/cpuidle/core.txt Documentation/cpuidle/sysfs.txt Documentation/cpuidle/driver.txt Documentation/cpuidle/governor.txt

Thanks Andrew's advice and Yanmin's review. Here is the new patch.

From: ShuoX Liu <[email protected]>

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.

Signed-off-by ShuoX Liu <[email protected]>

Reviewed-by Yanmin Zhang <[email protected]>

---
 Documentation/cpuidle/sysfs.txt  |    5 +++++
 drivers/cpuidle/cpuidle.c        |    1 +
 drivers/cpuidle/governors/menu.c |    5 ++++-
 drivers/cpuidle/sysfs.c          |   34 ++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h          |    1 +
 5 files changed, 45 insertions(+), 1 deletions(-)

Documentation/cpuidle/sysfs.txt +1 -0

index 50d7b16..635424f 100644
@@ -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
+-r--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

Documentation/cpuidle/sysfs.txt +1 -0

@@ -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
+-r--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

Documentation/cpuidle/sysfs.txt +3 -0

@@ -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
+-r--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
+-r--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)

drivers/cpuidle/cpuidle.c +1 -0

index 59f4261..7d66d3e 100644
@@ -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) {}

drivers/cpuidle/governors/menu.c +4 -1

index ad09526..5c17ca1 100644
@@ -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)

drivers/cpuidle/sysfs.c +34 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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 +232,19 @@ 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) \
+{ \
+	int value; \
+	sscanf(buf, "%d", &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 +267,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 +276,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 +285,7 @@ static struct attribute *cpuidle_state_default_attrs[] = {
 	&attr_power.attr,
 	&attr_usage.attr,
 	&attr_time.attr,
+	&attr_disable.attr,
 	NULL
 };
 

@@ -287,8 +307,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)

include/linux/cpuidle.h +0 -0

index 712abcc..eb150a5 100644

Hi,

Quoted 25 lines · ShuoX

Quoted 7 lines

Quoted 1 line

It's useful to help developers debug some issues.

It would help developers more if it was documented a bit. As it stands, they'd be lucky if they even knew it existed.

Looky:

Quoted 9 lines

akpm:/usr/src/linux-3.3-rc5> grep -ril cpuidle Documentation Documentation/trace/events-power.txt Documentation/ABI/testing/sysfs-devices-system-cpu Documentation/kernel-parameters.txt Documentation/00-INDEX Documentation/cpuidle/core.txt Documentation/cpuidle/sysfs.txt Documentation/cpuidle/driver.txt Documentation/cpuidle/governor.txt

Thanks Andrew's advice and Yanmin's review. Here is the new patch.

From: ShuoX Liu<[email protected]>

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.

Quoted 6 lines

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.

Enabling/Disabling particular C-states from sysfs entry is really helpful for cpuidle developers for general debugging and performance tuning for not just x86 but for all platforms that support cpuidle, like arm, powerpc etc .

Quoted 1 line

Quoted 2 lines

Signed-off-by ShuoX Liu<[email protected]>

Reviewed-by Yanmin Zhang<[email protected]>

Please update the documentation, rw fields are valid for disable flag /sys/devices/system/cpu/cpu2/cpuidle/state1 # ls -l total 0

Quoted 11 lines

static void cpuidle_state_sysfs_release(struct kobject *kobj)

include/linux/cpuidle.h +1 -0

index 712abcc..eb150a5 100644
@@ -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;
 

include/linux/cpuidle.h struct cpuidle_state { · line 51

Quoted 3 lines

include/linux/cpuidle.h +0 -0

       int (*enter)    (struct cpuidle_device *dev,
                       struct cpuidle_driver *drv,

Documentation/cpuidle/sysfs.txt +1 -0

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

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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, \

drivers/cpuidle/sysfs.c +2 -0

 			 struct cpuidle_state_usage *state_usage, char *buf) \

@@ -229,6 +232,19 @@ 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, \

Quoted 8 lines · ShuoX Liu

(quoted diff) +0 -0

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

...

Quoted 23 lines

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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) \

drivers/cpuidle/sysfs.c +2 -0

@@ -229,6 +232,19 @@ 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, \

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct · line 247

Quoted 10 lines

drivers/cpuidle/sysfs.c +10 -0

+             const char *buf, size_t size) \
+{ \
+     int value; \
+     sscanf(buf, "%d", &value); \
+     if (value) \
+             state->disable = 1; \
+     else \
+             state->disable = 0; \
+     return size; \
+}

Isn't this missing a check for capabilities?  Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Also, maybe it would be best to use one of the lib helpers to parse that value, so that it will be less annoying to userspace (trim blanks, complain if there is trailing junk after trimming, etc)?

--
 "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh
_______________________________________________
linux-pm mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-pm

Hello,

Quoted 50 lines · Henrique de Moraes Holschuh

Quoted 8 lines · ShuoX Liu

Documentation/cpuidle/sysfs.txt +1 -0

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

...

Quoted 23 lines

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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) \

drivers/cpuidle/sysfs.c +2 -0

@@ -229,6 +232,19 @@ 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, \

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct · line 247

Quoted 10 lines

drivers/cpuidle/sysfs.c +10 -0

+             const char *buf, size_t size) \
+{ \
+     int value; \
+     sscanf(buf, "%d", &value); \
+     if (value) \
+             state->disable = 1; \
+     else \
+             state->disable = 0; \
+     return size; \
+}

Isn't this missing a check for capabilities? Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Quoted 3 lines

Also, maybe it would be best to use one of the lib helpers to parse that value, so that it will be less annoying to userspace (trim blanks, complain if there is trailing junk after trimming, etc)?

I may be jumping the thread in the middle but, if it is for debug purposes, as states the subject, shouldn't this entry go to debugfs instead of sysfs? I know cpuidle has all the infrastructure there to simply add another sysfs entry, but if the intent is to create a debug capability, then I'd say it fits under debugfs instead. Adding Greg KH here, as I suppose he may have strong opinion on using sysfs for debugging.

Of course, if you are targeting user space control over C states on production systems, then it's a different thing then.

Quoted 59 lines · Eduardo

Hello,

Quoted 50 lines · Henrique de Moraes Holschuh

Quoted 8 lines · ShuoX Liu

Documentation/cpuidle/sysfs.txt +1 -0

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

...

Quoted 23 lines

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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) \

drivers/cpuidle/sysfs.c +2 -0

@@ -229,6 +232,19 @@ 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, \

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct · line 247

Quoted 10 lines

drivers/cpuidle/sysfs.c +10 -0

+             const char *buf, size_t size) \
+{ \
+     int value; \
+     sscanf(buf, "%d", &value); \
+     if (value) \
+             state->disable = 1; \
+     else \
+             state->disable = 0; \
+     return size; \
+}

Isn't this missing a check for capabilities? Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Quoted 3 lines

Also, maybe it would be best to use one of the lib helpers to parse that value, so that it will be less annoying to userspace (trim blanks, complain if there is trailing junk after trimming, etc)?

Quoted 7 lines

I may be jumping the thread in the middle but, if it is for debug purposes, as states the subject, shouldn't this entry go to debugfs instead of sysfs? I know cpuidle has all the infrastructure there to simply add another sysfs entry, but if the intent is to create a debug capability, then I'd say it fits under debugfs instead. Adding Greg KH here, as I suppose he may have strong opinion on using sysfs for debugging.

Thanks for the comments.

IMHO, all entries under cpuidle directory are for debug purpose. End users shouldn't care about them. If we rewrite codes around all the entries, I strongly agree that we need move them to debugfs.

Here, we just add a new entry under same directory. If we create it under debugfs, we need create the similar directory tree, which is a duplicate effort. In addition, users might be confused that why we separate the entries under sysfs and debugfs.

Quoted 67 lines · Yanmin Zhang

Quoted 59 lines · Eduardo

Hello,

Quoted 50 lines · Henrique de Moraes Holschuh

Quoted 8 lines · ShuoX Liu

Documentation/cpuidle/sysfs.txt +1 -0

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

...

Quoted 23 lines

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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) \

drivers/cpuidle/sysfs.c +2 -0

@@ -229,6 +232,19 @@ 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, \

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct · line 247

Quoted 10 lines

drivers/cpuidle/sysfs.c +10 -0

+             const char *buf, size_t size) \
+{ \
+     int value; \
+     sscanf(buf, "%d", &value); \
+     if (value) \
+             state->disable = 1; \
+     else \
+             state->disable = 0; \
+     return size; \
+}

Isn't this missing a check for capabilities? Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Quoted 3 lines

Also, maybe it would be best to use one of the lib helpers to parse that value, so that it will be less annoying to userspace (trim blanks, complain if there is trailing junk after trimming, etc)?

Quoted 7 lines

I may be jumping the thread in the middle but, if it is for debug purposes, as states the subject, shouldn't this entry go to debugfs instead of sysfs? I know cpuidle has all the infrastructure there to simply add another sysfs entry, but if the intent is to create a debug capability, then I'd say it fits under debugfs instead. Adding Greg KH here, as I suppose he may have strong opinion on using sysfs for debugging.

Quoted 5 lines

Thanks for the comments.

IMHO, all entries under cpuidle directory are for debug purpose. End users shouldn't care about them. If we rewrite codes around all the entries, I strongly agree that we need move them to debugfs.

I totally agree, they all need to move out of sysfs.

Quoted 3 lines

Here, we just add a new entry under same directory. If we create it under debugfs, we need create the similar directory tree, which is a duplicate effort. In addition, users might be confused that why we separate the entries under sysfs and debugfs.

They should all be moved there, that will remove any confusion :)

Quoted 74 lines · Greg KH

Quoted 67 lines · Yanmin Zhang

Quoted 59 lines · Eduardo

Hello,

Quoted 50 lines · Henrique de Moraes Holschuh

Quoted 8 lines · ShuoX Liu

Documentation/cpuidle/sysfs.txt +1 -0

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

...

Quoted 23 lines

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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) \

drivers/cpuidle/sysfs.c +2 -0

@@ -229,6 +232,19 @@ 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, \

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct · line 247

Quoted 10 lines

drivers/cpuidle/sysfs.c +10 -0

+             const char *buf, size_t size) \
+{ \
+     int value; \
+     sscanf(buf, "%d", &value); \
+     if (value) \
+             state->disable = 1; \
+     else \
+             state->disable = 0; \
+     return size; \
+}

Isn't this missing a check for capabilities? Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Quoted 3 lines

Also, maybe it would be best to use one of the lib helpers to parse that value, so that it will be less annoying to userspace (trim blanks, complain if there is trailing junk after trimming, etc)?

Quoted 7 lines

I may be jumping the thread in the middle but, if it is for debug purposes, as states the subject, shouldn't this entry go to debugfs instead of sysfs? I know cpuidle has all the infrastructure there to simply add another sysfs entry, but if the intent is to create a debug capability, then I'd say it fits under debugfs instead. Adding Greg KH here, as I suppose he may have strong opinion on using sysfs for debugging.

Quoted 5 lines

Thanks for the comments.

IMHO, all entries under cpuidle directory are for debug purpose. End users shouldn't care about them. If we rewrite codes around all the entries, I strongly agree that we need move them to debugfs.

Quoted 7 lines

I totally agree, they all need to move out of sysfs.

Quoted 3 lines

Here, we just add a new entry under same directory. If we create it under debugfs, we need create the similar directory tree, which is a duplicate effort. In addition, users might be confused that why we separate the entries under sysfs and debugfs.

They should all be moved there, that will remove any confusion :)

Greg,

Sorry. I might mislead you.

Basically, we could move all the entries of cpuidle from sysfs to debugfs. But such moving would change KBI. There might be many scripts used by end users to parse the data. If we change them to debugfs, the scripts wouldn't work and users would complain.

What's your opinion about the KBI consistence?

Quoted 116 lines · Yanmin Zhang

Quoted 97 lines · Greg KH

Quoted 84 lines · Yanmin Zhang

Quoted 74 lines · Greg KH

Quoted 67 lines · Yanmin Zhang

Quoted 59 lines · Eduardo

Hello,

Quoted 50 lines · Henrique de Moraes Holschuh

Quoted 8 lines · ShuoX Liu

(quoted diff) +1 -0

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

...

Quoted 23 lines

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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) \

drivers/cpuidle/sysfs.c +2 -0

@@ -229,6 +232,19 @@ 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, \

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct · line 247

Quoted 10 lines

drivers/cpuidle/sysfs.c +10 -0

+             const char *buf, size_t size) \
+{ \
+     int value; \
+     sscanf(buf, "%d",&value); \
+     if (value) \
+             state->disable = 1; \
+     else \
+             state->disable = 0; \
+     return size; \
+}

Isn't this missing a check for capabilities? Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Quoted 3 lines

Also, maybe it would be best to use one of the lib helpers to parse that value, so that it will be less annoying to userspace (trim blanks, complain if there is trailing junk after trimming, etc)?

Quoted 7 lines

I may be jumping the thread in the middle but, if it is for debug purposes, as states the subject, shouldn't this entry go to debugfs instead of sysfs? I know cpuidle has all the infrastructure there to simply add another sysfs entry, but if the intent is to create a debug capability, then I'd say it fits under debugfs instead. Adding Greg KH here, as I suppose he may have strong opinion on using sysfs for debugging.

Quoted 5 lines

Thanks for the comments.

IMHO, all entries under cpuidle directory are for debug purpose. End users shouldn't care about them. If we rewrite codes around all the entries, I strongly agree that we need move them to debugfs.

Quoted 7 lines

I totally agree, they all need to move out of sysfs.

Quoted 3 lines

Here, we just add a new entry under same directory. If we create it under debugfs, we need create the similar directory tree, which is a duplicate effort. In addition, users might be confused that why we separate the entries under sysfs and debugfs.

They should all be moved there, that will remove any confusion :)

Greg,

Quoted 8 lines

Sorry. I might mislead you.

Basically, we could move all the entries of cpuidle from sysfs to debugfs. But such moving would change KBI. There might be many scripts used by end users to parse the data. If we change them to debugfs, the scripts wouldn't work and users would complain.

What's your opinion about the KBI consistence?

These files all are debugging files, right? So they should be moved, especially as the violate the "one value per file" rule of sysfs.

Quoted 6 lines

Do you know of any tools using these files? I have never heard of them, and I was told we should move these files years ago. So I don't think there should be any api issues.

But, if there are, we need to know what they are, and work to preserve them. The only way to find out is to move them :)

We would move all cpuidle entries to debugfs firstly.
1) Create the similar directory tree on debugfs: cpu/cpuXXX, but just have cpuidle subdirectory;
2) Move our cpuidle->disable patch to debugfs;
sysfs cpuXXX has other sub directories. We don't move them this time.

The new directory under debugfs becomes:
cpu---cpu0---cpuidle

	|
	|
       cpu1---cpuidle
	|

Quoted 1 line

Is it ok?

Looks fine to me.

greg k-h

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.

I created a series to do this.

[PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs. [PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state for debug purpose.
[PATCH 3/3] cpupower: Update the cpupower tool for new debugfs entries of cpuidle.

Below are patches.

Quoted 8 lines · ShuoX Liu

I created a series to do this.

[PATCH 1/3] cpuidle: Move cpuidle sysfs entry of each cpu to debugfs. [PATCH 2/3] cpuidle: Add a debugfs entry to disable specific C state for debug purpose.
[PATCH 3/3] cpupower: Update the cpupower tool for new debugfs entries of cpuidle.

Quoted 1 line

Below are patches.

The patch is line-wrapped and can not be applied :(

Quoted 4 lines

drivers/cpuidle/debugfs.c new file +1 -0

@@ -0,0 +1,176 @@
+/*

drivers/cpuidle/debugfs.c line 7

Quoted 6 lines

drivers/cpuidle/debugfs.c new file +6 -0

+ * debugfs.c - debugfs support
+ *
+ * (C) 2006-2007 Shaohua Li <[email protected]>
+ * (C) 2012 ShuoX Liu <[email protected]>
+ *
+ * This code is licenced under the GPL.

What version of the GPL?

thanks,

greg k-h

Do you know of any tools using these files? I have never heard of them, and I was told we should move these files years ago. So I don't think there should be any api issues.

Quoted 3 lines · Greg KH

Do you know of any tools using these files? I have never heard of them, and I was told we should move these files years ago. So I don't think there should be any api issues.

powertop uses them.

Quoted 6 lines · Mark Brown

Quoted 3 lines · Greg KH

Do you know of any tools using these files? I have never heard of them, and I was told we should move these files years ago. So I don't think there should be any api issues.

Quoted 1 line

powertop uses them.

Ok, then we can't move them all.

We should then just move the ones that have multiple lines, as I'm pretty sure powertop doesn't use them, right?

Quoted 11 lines · Greg KH

Quoted 7 lines · Mark Brown

Quoted 3 lines · Greg KH

Do you know of any tools using these files? I have never heard of them, and I was told we should move these files years ago. So I don't think there should be any api issues.

powertop uses them.

Ok, then we can't move them all.

Quoted 2 lines

We should then just move the ones that have multiple lines, as I'm pretty sure powertop doesn't use them, right?

All sys files under cpu/cpuXXX/cpuidle have single line. If we move some files to debugfs and keep others under sysfs, users might be confused.

Should we go back to the 1st version which just adds the new entry to sysfs?

Quoted 14 lines · Yanmin Zhang

Quoted 13 lines · Greg KH

Quoted 7 lines · Mark Brown

Quoted 3 lines · Greg KH

Do you know of any tools using these files? I have never heard of them, and I was told we should move these files years ago. So I don't think there should be any api issues.

powertop uses them.

Ok, then we can't move them all.

We should then just move the ones that have multiple lines, as I'm pretty sure powertop doesn't use them, right?

Quoted 5 lines

All sys files under cpu/cpuXXX/cpuidle have single line. If we move some files to debugfs and keep others under sysfs, users might be confused.

Should we go back to the 1st version which just adds the new entry to sysfs?

Wait, I thought this whole thing started when we wanted to move the files that had multiple lines out of sysfs?

If none of these do, and they all are being used by tools already, then fine, they should stay.

But for some reason, I thought there was a problem here. Perhaps that was in the cpufreq code?

Quoted 15 lines · Greg KH

Quoted 14 lines · Yanmin Zhang

Quoted 13 lines · Greg KH

Quoted 7 lines · Mark Brown

Quoted 3 lines · Greg KH

Do you know of any tools using these files? I have never heard of them, and I was told we should move these files years ago. So I don't think there should be any api issues.

powertop uses them.

Ok, then we can't move them all.

We should then just move the ones that have multiple lines, as I'm pretty sure powertop doesn't use them, right?

Quoted 8 lines

Quoted 5 lines

All sys files under cpu/cpuXXX/cpuidle have single line. If we move some files to debugfs and keep others under sysfs, users might be confused.

Should we go back to the 1st version which just adds the new entry to sysfs?

Wait, I thought this whole thing started when we wanted to move the files that had multiple lines out of sysfs?

No. Liu Shuo's patch adds a new entry under cpu/cpuXXX/cpuidle and users can disable specific C state.
A gentleman raised that if we should move it to debugfs, then you suggested to move all files under cpu/cpuXXX/cpuidle to debugfs.

Quoted 3 lines

If none of these do, and they all are being used by tools already, then fine, they should stay.

Agree.

Quoted 1 line

Quoted 2 lines

But for some reason, I thought there was a problem here. Perhaps that was in the cpufreq code?

I checked cpufreq quickly. Every file has single-line, but some have multiple-fields.

We would send a new patch based on sysfs as the new entry has single line.

From: ShuoX Liu <[email protected]>

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.

Signed-off-by ShuoX Liu <[email protected]>

Reviewed-by Yanmin Zhang <[email protected]>

Reviewed-and-Tested-by Deepthi Dharwar <[email protected]>

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

Documentation/cpuidle/sysfs.txt +5 -0

index 50d7b16..9d28a34 100644
@@ -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)

drivers/cpuidle/cpuidle.c +1 -0

index 59f4261..7d66d3e 100644
@@ -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) {}

drivers/cpuidle/governors/menu.c +4 -1

index ad09526..5c17ca1 100644
@@ -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)

drivers/cpuidle/sysfs.c +37 -0

index 3fe41fe..f0fe0f5 100644
@@ -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)

include/linux/cpuidle.h +1 -0

index 712abcc..eb150a5 100644
@@ -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,

From: ShuoX Liu <[email protected]>

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.

...

drivers/cpuidle/sysfs.c +2 -0

+#define define_one_state_rw(_name, show, store) \
+static struct cpuidle_state_attr attr_##_name = __ATTR(_name, 0644, show, store)
 

drivers/cpuidle/sysfs.c +7 -0

+	kstrtol(buf, 0, &value); \
+	if (value) \
+		state->disable = 1; \
+	else \
+		state->disable = 0; \
+	return size; \
+}
 
...

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct cpuidle_state *state, \ · line 242

Quoted 7 lines

drivers/cpuidle/sysfs.c +7 -0

+#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; \

Is the capability check required? The 0644 permissions aren't sufficient?

From: ShuoX Liu <[email protected]>

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.

...

(quoted diff) +4 -0

+#define define_store_state_function(_name) \
+static ssize_t store_state_##_name(struct cpuidle_state *state, \
+		const char *buf, size_t size) \
+{ \

Quoted 8 lines · ShuoX Liu

Documentation/cpuidle/sysfs.txt +1 -0

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

...

Quoted 21 lines

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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, \

drivers/cpuidle/sysfs.c +0 -0

 			 struct cpuidle_state_usage *state_usage, char *buf) \

@@ -229,6 +232,19 @@ static ssize_t show_state_##_name(struct
 cpuidle_state *state, \
 	return sprintf(buf, "%u\n", state->_name);\
 }
 

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct · line 247

Quoted 12 lines

drivers/cpuidle/sysfs.c +12 -0

+#define define_store_state_function(_name) \
+static ssize_t store_state_##_name(struct cpuidle_state *state, \
+		const char *buf, size_t size) \
+{ \
+	int value; \
+	sscanf(buf, "%d", &value); \
+	if (value) \
+		state->disable = 1; \
+	else \
+		state->disable = 0; \
+	return size; \
+}

Isn't this missing a check for capabilities? Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Also, maybe it would be best to use one of the lib helpers to parse that value, so that it will be less annoying to userspace (trim blanks, complain if there is trailing junk after trimming, etc)?

Quoted 33 lines · Henrique de Moraes Holschuh

Quoted 8 lines · ShuoX Liu

Documentation/cpuidle/sysfs.txt +1 -0

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

...

Quoted 21 lines

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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, \

drivers/cpuidle/sysfs.c +0 -0

 			 struct cpuidle_state_usage *state_usage, char *buf) \

@@ -229,6 +232,19 @@ static ssize_t show_state_##_name(struct
 cpuidle_state *state, \
 	return sprintf(buf, "%u\n", state->_name);\
 }
 

Quoted 16 lines

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct · line 247

Quoted 12 lines

drivers/cpuidle/sysfs.c +12 -0

+#define define_store_state_function(_name) \
+static ssize_t store_state_##_name(struct cpuidle_state *state, \
+		const char *buf, size_t size) \
+{ \
+	int value; \
+	sscanf(buf, "%d", &value); \
+	if (value) \
+		state->disable = 1; \
+	else \
+		state->disable = 0; \
+	return size; \
+}

Isn't this missing a check for capabilities? Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Sorry. Could you elaborate it?

Quoted 3 lines

Also, maybe it would be best to use one of the lib helpers to parse that value, so that it will be less annoying to userspace (trim blanks, complain if there is trailing junk after trimming, etc)?

We would use strict_strtol to parse the value in next version.

Quoted 50 lines · Yanmin Zhang

Quoted 49 lines · Henrique de Moraes Holschuh

Quoted 8 lines · ShuoX Liu

Documentation/cpuidle/sysfs.txt +1 -0

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

...

Quoted 23 lines

drivers/cpuidle/sysfs.c +3 -0

index 3fe41fe..1eae29a 100644
@@ -222,6 +222,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, \

drivers/cpuidle/sysfs.c +2 -0

 			 struct cpuidle_state_usage *state_usage, char *buf) \

@@ -229,6 +232,19 @@ 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, \

drivers/cpuidle/sysfs.c static ssize_t show_state_##_name(struct · line 247

Quoted 10 lines

drivers/cpuidle/sysfs.c +10 -0

+		const char *buf, size_t size) \
+{ \
+	int value; \
+	sscanf(buf, "%d", &value); \
+	if (value) \
+		state->disable = 1; \
+	else \
+		state->disable = 0; \
+	return size; \
+}

Isn't this missing a check for capabilities? Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Quoted 1 line

Sorry. Could you elaborate it?

Sure. Should any user be able to disable a C state, therefore causing the system to consume more power?

I am pretty sure the answer is NO, in which case you should check for the appropriate user credentials before you allow a write to these "debug" controls to succeed. "capability" here is one of the CAP_* capabilities tested through capable(), which are supposed to limit even root.

Quoted 5 lines

Quoted 4 lines

Also, maybe it would be best to use one of the lib helpers to parse that value, so that it will be less annoying to userspace (trim blanks, complain if there is trailing junk after trimming, etc)?

We would use strict_strtol to parse the value in next version.

Thanks!

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+static ssize_t store_state_##_name(struct cpuidle_state *state, \
+		const char *buf, size_t size) \
+{ \
+	int value; \
+	sscanf(buf, "%d", &value); \
+	if (value) \
+		state->disable = 1; \
+	else \
+		state->disable = 0; \
+	return size; \
+}

Isn't this missing a check for capabilities? Disabling cpuidle states is not something random Joe (and IMHO that does mean random capability- restricted Joe root) should be doing...

Quoted 4 lines

Quoted 1 line

Sorry. Could you elaborate it?

Sure. Should any user be able to disable a C state, therefore causing the system to consume more power?

Here we use the simple way to check access. Only root could change it.

Quoted 5 lines

I am pretty sure the answer is NO, in which case you should check for the appropriate user credentials before you allow a write to these "debug" controls to succeed. "capability" here is one of the CAP_* capabilities tested through capable(), which are supposed to limit even root.

We would add below check.

        if (!capable(CAP_SYS_ADMIN))
                return -EPERM;