Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states
On Wed, 12 Jan 2011 17:25:12 EST, Len Brown said:
But by sysfs convention a separate file must be used
if two data are passed to userspace which is the case here.
what two data?
It is fine for a string to include space characters.
I think Thomas is concerned that although when you actually read a /sys
file, you know it's one string, that fact can get easily lost and cause issues
down the road. Consider this code:
foo=`cat /sys/some/file`
bar=`cat /sys/other/file`
baz=`cat /sys/third/file`
echo $foo $bar $baz | awk '{print $2 $3}'
Suddenly your output isn't what you expected...
pgpoKvq0hG1oS.pgp
Description: PGP signature
Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states
On Wednesday 12 January 2011 07:56:45 Len Brown wrote: I'm not fond of inventing a new 3-character abbreviation field for every state because display tools can't handle the existing 16-character name field. I am also not fond of it, but it's a sane and easy solution and appropriate for this issue. If the display tools can only handle 3 characters, then why not have them simply use the 1st 3 characters of the existing name field? You mean use: C3 NHM instead of NHM-C3 for intel_idle? Then userspace has to parse the two informations glued together, get rid of the whitespace, etc. But by sysfs convention a separate file must be used if two data are passed to userspace which is the case here. If that is not unique, then re-arrange the strings so that it is unique... See above, it would unnecessarily make life hard for userspace. Afaik sysfs entries do not consume resources as long as they do not get accessed. Of course the ACPI part of this patch will not apply, as it depends on patch 1 in this series, which was erroneous. For ACPI, the existing name field is already fine, as C%d fits into 3 characters. For acpi_idle only it would work, but this is (nearly) the only one. For example for sh arch: name: SuperH SH SuperH contains two data and should get split up into: name: SuperH abbr: SH By convention the first characters of description could be used, but you would again end up in userspace parsing and double info in one sysfs file. I hope that's enough for convincing, I'd really like to go the .abbr way. Do you give me an acked-by for that one? Thanks, Thomas -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states
If the display tools can only handle 3 characters, then why not have them simply use the 1st 3 characters of the existing name field? You mean use: C3 NHM instead of NHM-C3 for intel_idle? Yes. is there a reason that would be a problem? If only 3 characters are useful, then I'd rather see you simly do this: #define CPUIDLE_NAME_LEN 3 Then userspace has to parse the two informations glued together, get rid of the whitespace, etc. what two informations? why is kernel bloat the instrument of choice to avoid the expense of string truncation in user-space tools? But by sysfs convention a separate file must be used if two data are passed to userspace which is the case here. what two data? It is fine for a string to include space characters. If that is not unique, then re-arrange the strings so that it is unique... See above, it would unnecessarily make life hard for userspace. Afaik sysfs entries do not consume resources as long as they do not get accessed. they clutter the source code, they create an additional name for something that arguably already has 3 names -- the state #, the name, and the description. I'd rather delete a few of the fields than add a 4th... Of course the ACPI part of this patch will not apply, as it depends on patch 1 in this series, which was erroneous. For ACPI, the existing name field is already fine, as C%d fits into 3 characters. For acpi_idle only it would work, but this is (nearly) the only one. For example for sh arch: name: SuperH SH SuperH contains two data and should get split up into: name: SuperH abbr: SH acpi/arch/sh/kernel/cpu/shmobile/cpuidle.c already uses state-name = C0, C1, C2. Just use them and be done. By convention the first characters of description could be used, but you would again end up in userspace parsing and double info in one sysfs file. I hope that's enough for convincing, I'd really like to go the .abbr way. Do you give me an acked-by for that one? No. We should not invent an additional abbreviation field. The tools should use the existing name field. If the existing name field is not sufficient, then the states should simply be enumerated and numbers be shown in the GUI. thanks, Len Brown, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states
On Wednesday 12 January 2011 23:25:12 Len Brown wrote: If the display tools can only handle 3 characters, then why not have them simply use the 1st 3 characters of the existing name field? You mean use: C3 NHM instead of NHM-C3 for intel_idle? Yes. is there a reason that would be a problem? A much more elegant solution came to my mind: I'll rewrite the perf timechart patch to do: Title: C-stateName1 [1] , C-stateName2 [2] and later I will refer to the states by 1, 2, ... Eventually a: Description: [1] C-stateDescription1 [2] C-stateDescription2 ... can be put somewhere as well. Please ignore this patch. Stupid that I didn't think of that in the first place. Thanks, Thomas -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states
I'm not fond of inventing a new 3-character abbreviation field for every state because display tools can't handle the existing 16-character name field. If the display tools can only handle 3 characters, then why not have them simply use the 1st 3 characters of the existing name field? If that is not unique, then re-arrange the strings so that it is unique... Of course the ACPI part of this patch will not apply, as it depends on patch 1 in this series, which was erroneous. For ACPI, the existing name field is already fine, as C%d fits into 3 characters. thanks, Len Brown, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states
and fill name, description and newly introduced abbrevation
consistently (always use snprintf) in all cpuidle drivers.
This is mainly for perf timechart. It draws vector graphics
pictures of sleep/idle state usage catching perf cpu_idle events.
The string used for the idle state must not exceed 3 chars or
you can't see much in these pictures.
The name could get used in the title, the introduced abbrevations
inside of the picture and the text must therefore be rather short.
Signed-off-by: Thomas Renninger tr...@suse.de
CC: l...@kernel.org
CC: linux-a...@vger.kernel.org
CC: linux...@lists.linux-foundation.org
CC: Arjan van de Ven ar...@linux.intel.com
CC: Ingo Molnar mi...@elte.hu
CC: Frederic Weisbecker fweis...@gmail.com
CC: linux-ker...@vger.kernel.org
CC: linux-omap@vger.kernel.org
---
arch/arm/mach-at91/cpuidle.c | 12
arch/arm/mach-davinci/cpuidle.c | 13 +
arch/arm/mach-kirkwood/cpuidle.c | 12
arch/arm/mach-omap2/cpuidle34xx.c |3 ++-
arch/sh/kernel/cpu/shmobile/cpuidle.c | 19 +++
drivers/acpi/processor_idle.c |3 +++
drivers/cpuidle/cpuidle.c |1 +
drivers/cpuidle/sysfs.c |3 +++
drivers/idle/intel_idle.c | 11 +++
include/linux/cpuidle.h |2 ++
10 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index 1cfeac1..6cdeb42 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -73,16 +73,20 @@ static int at91_init_cpuidle(void)
device-states[0].exit_latency = 1;
device-states[0].target_residency = 1;
device-states[0].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[0].name, WFI);
- strcpy(device-states[0].desc, Wait for interrupt);
+ snprintf(device-states[0].name, CPUIDLE_NAME_LEN, WFI);
+ snprintf(device-states[0].desc, CPUIDLE_DESC_LEN,
+Wait for interrupt);
+ snprintf(device-states[0].abbr, CPUIDLE_ABBR_LEN, W);
/* Wait for interrupt and RAM self refresh state */
device-states[1].enter = at91_enter_idle;
device-states[1].exit_latency = 10;
device-states[1].target_residency = 1;
device-states[1].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[1].name, RAM_SR);
- strcpy(device-states[1].desc, WFI and RAM Self Refresh);
+ snprintf(device-states[1].name, CPUIDLE_NAME_LEN, RAM SR);
+ snprintf(device-states[1].desc, CPUIDLE_DESC_LEN,
+WFI and RAM Self Refresh);
+ snprintf(device-states[1].abbr, CPUIDLE_ABBR_LEN, WSR);
if (cpuidle_register_device(device)) {
printk(KERN_ERR at91_init_cpuidle: Failed registering\n);
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
index bd59f31..42ad2d6 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -127,16 +127,21 @@ static int __init davinci_cpuidle_probe(struct
platform_device *pdev)
device-states[0].exit_latency = 1;
device-states[0].target_residency = 1;
device-states[0].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[0].name, WFI);
- strcpy(device-states[0].desc, Wait for interrupt);
+ snprintf(device-states[0].name, CPUIDLE_NAME_LEN, WFI);
+ snprintf(device-states[0].desc, CPUIDLE_DESC_LEN,
+Wait for interrupt);
+ snprintf(device-states[0].abbr, CPUIDLE_ABBR_LEN, W);
/* Wait for interrupt and DDR self refresh state */
device-states[1].enter = davinci_enter_idle;
device-states[1].exit_latency = 10;
device-states[1].target_residency = 1;
device-states[1].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[1].name, DDR SR);
- strcpy(device-states[1].desc, WFI and DDR Self Refresh);
+ snprintf(device-states[1].name, CPUIDLE_NAME_LEN, RAM SR);
+ snprintf(device-states[1].desc, CPUIDLE_DESC_LEN,
+WFI and RAM Self Refresh);
+ snprintf(device-states[1].abbr, CPUIDLE_ABBR_LEN, WSR);
+
if (pdata-ddr2_pdown)
davinci_states[1].flags |= DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN;
cpuidle_set_statedata(device-states[1], davinci_states[1]);
diff --git a/arch/arm/mach-kirkwood/cpuidle.c b/arch/arm/mach-kirkwood/cpuidle.c
index f68d33f..48eaabb 100644
--- a/arch/arm/mach-kirkwood/cpuidle.c
+++ b/arch/arm/mach-kirkwood/cpuidle.c
@@ -75,16 +75,20 @@ static int kirkwood_init_cpuidle(void)
device-states[0].exit_latency = 1;
device-states[0].target_residency = 1;
device-states[0].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[0].name, WFI);
- strcpy(device-states[0].desc, Wait for interrupt);
+ snprintf(device-states[0].name, CPUIDLE_NAME_LEN, WFI);
+ snprintf(device-states[0].desc,
Re: [PATCH 4/9] cpuidle: Introduce .abbr (abbrevation) for cpuidle states
Thomas Renninger tr...@suse.de writes:
and fill name, description and newly introduced abbrevation
consistently (always use snprintf) in all cpuidle drivers.
This is mainly for perf timechart. It draws vector graphics
pictures of sleep/idle state usage catching perf cpu_idle events.
The string used for the idle state must not exceed 3 chars or
you can't see much in these pictures.
The name could get used in the title, the introduced abbrevations
inside of the picture and the text must therefore be rather short.
Signed-off-by: Thomas Renninger tr...@suse.de
CC: l...@kernel.org
CC: linux-a...@vger.kernel.org
CC: linux...@lists.linux-foundation.org
CC: Arjan van de Ven ar...@linux.intel.com
CC: Ingo Molnar mi...@elte.hu
CC: Frederic Weisbecker fweis...@gmail.com
CC: linux-ker...@vger.kernel.org
CC: linux-omap@vger.kernel.org
---
arch/arm/mach-at91/cpuidle.c | 12
arch/arm/mach-davinci/cpuidle.c | 13 +
arch/arm/mach-kirkwood/cpuidle.c | 12
arch/arm/mach-omap2/cpuidle34xx.c |3 ++-
arch/sh/kernel/cpu/shmobile/cpuidle.c | 19 +++
drivers/acpi/processor_idle.c |3 +++
drivers/cpuidle/cpuidle.c |1 +
drivers/cpuidle/sysfs.c |3 +++
drivers/idle/intel_idle.c | 11 +++
include/linux/cpuidle.h |2 ++
10 files changed, 58 insertions(+), 21 deletions(-)
For the davinci OMAP changes,
Acked-by: Kevin Hilman khil...@ti.com
diff --git a/arch/arm/mach-at91/cpuidle.c b/arch/arm/mach-at91/cpuidle.c
index 1cfeac1..6cdeb42 100644
--- a/arch/arm/mach-at91/cpuidle.c
+++ b/arch/arm/mach-at91/cpuidle.c
@@ -73,16 +73,20 @@ static int at91_init_cpuidle(void)
device-states[0].exit_latency = 1;
device-states[0].target_residency = 1;
device-states[0].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[0].name, WFI);
- strcpy(device-states[0].desc, Wait for interrupt);
+ snprintf(device-states[0].name, CPUIDLE_NAME_LEN, WFI);
+ snprintf(device-states[0].desc, CPUIDLE_DESC_LEN,
+ Wait for interrupt);
+ snprintf(device-states[0].abbr, CPUIDLE_ABBR_LEN, W);
/* Wait for interrupt and RAM self refresh state */
device-states[1].enter = at91_enter_idle;
device-states[1].exit_latency = 10;
device-states[1].target_residency = 1;
device-states[1].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[1].name, RAM_SR);
- strcpy(device-states[1].desc, WFI and RAM Self Refresh);
+ snprintf(device-states[1].name, CPUIDLE_NAME_LEN, RAM SR);
+ snprintf(device-states[1].desc, CPUIDLE_DESC_LEN,
+ WFI and RAM Self Refresh);
+ snprintf(device-states[1].abbr, CPUIDLE_ABBR_LEN, WSR);
if (cpuidle_register_device(device)) {
printk(KERN_ERR at91_init_cpuidle: Failed registering\n);
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c
index bd59f31..42ad2d6 100644
--- a/arch/arm/mach-davinci/cpuidle.c
+++ b/arch/arm/mach-davinci/cpuidle.c
@@ -127,16 +127,21 @@ static int __init davinci_cpuidle_probe(struct
platform_device *pdev)
device-states[0].exit_latency = 1;
device-states[0].target_residency = 1;
device-states[0].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[0].name, WFI);
- strcpy(device-states[0].desc, Wait for interrupt);
+ snprintf(device-states[0].name, CPUIDLE_NAME_LEN, WFI);
+ snprintf(device-states[0].desc, CPUIDLE_DESC_LEN,
+ Wait for interrupt);
+ snprintf(device-states[0].abbr, CPUIDLE_ABBR_LEN, W);
/* Wait for interrupt and DDR self refresh state */
device-states[1].enter = davinci_enter_idle;
device-states[1].exit_latency = 10;
device-states[1].target_residency = 1;
device-states[1].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[1].name, DDR SR);
- strcpy(device-states[1].desc, WFI and DDR Self Refresh);
+ snprintf(device-states[1].name, CPUIDLE_NAME_LEN, RAM SR);
+ snprintf(device-states[1].desc, CPUIDLE_DESC_LEN,
+ WFI and RAM Self Refresh);
+ snprintf(device-states[1].abbr, CPUIDLE_ABBR_LEN, WSR);
+
if (pdata-ddr2_pdown)
davinci_states[1].flags |= DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN;
cpuidle_set_statedata(device-states[1], davinci_states[1]);
diff --git a/arch/arm/mach-kirkwood/cpuidle.c
b/arch/arm/mach-kirkwood/cpuidle.c
index f68d33f..48eaabb 100644
--- a/arch/arm/mach-kirkwood/cpuidle.c
+++ b/arch/arm/mach-kirkwood/cpuidle.c
@@ -75,16 +75,20 @@ static int kirkwood_init_cpuidle(void)
device-states[0].exit_latency = 1;
device-states[0].target_residency = 1;
device-states[0].flags = CPUIDLE_FLAG_TIME_VALID;
- strcpy(device-states[0].name, WFI);
- strcpy(device-states[0].desc, Wait for
