x86/cpu: sort cpuinfo flags

PATCH In review Dave Hansen 6 participants 12 messages Dec 19, 2018 – Dec 22, 2018 x86

lore

Share & tools

lore

Dave Hansen

author maintainer

2018-12-19 19:55 UTC #0 lore raw

PATCH 1/1 x86/cpu: sort cpuinfo flags

From: Dave Hansen <[email protected]>

I frequently find myself contemplating my life choices as I try to find 3-character entries in the 1,000-character, unsorted "flags:" field of /proc/cpuinfo.

Sort that field, giving me hours back in my day.

This eats up ~1200 bytes (NCAPINTS*2*32) of space for the sorted array. I used an 'unsigned short' to use 1/4 of the space on 64-bit that would have been needed had pointers been used in the array.

An alternatve, requiring no array, would be to do the sort at runtime, but it seems ridiculous for a 500-cpu system to do 500 sorts for each 'cat /proc/cpuinfo'.

Another would be to just cache the *string* that results from this, which would be even faster at runtime because it could do a single seq_printf() and would consume less space. But, that would require a bit more infrastructure to make sure that the produced string never changed and was consistent across all CPUs, unless we want to store a string per 'struct cpuinfo_x86'.

Signed-off-by Dave Hansen <[email protected]>

Cc [email protected]

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Jia Zhang <[email protected]>

Cc "Gustavo A. R. Silva" <[email protected]>

Cc [email protected]

---

 b/arch/x86/kernel/cpu/proc.c |   80 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 6 deletions(-)

arch/x86/kernel/cpu/proc.c renamed from arch/x86/kernel/cpu/proc.c~x86-sorted-flags +74 -6

diff -puN arch/x86/kernel/cpu/proc.c~x86-sorted-flags arch/x86/kernel/cpu/proc.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/smp.h>
+#include <linux/sort.h>
 #include <linux/timex.h>
 #include <linux/string.h>
 #include <linux/seq_file.h>
+#include <linux/spinlock.h>
 #include <linux/cpufreq.h>
 
 #include "cpu.h"

@@ -54,6 +56,76 @@ static void show_cpuinfo_misc(struct seq
 }
 #endif
 
+#define X86_NR_CAPS	(32*NCAPINTS)
+/*
+ * x86_cap_flags[] is an array of string pointers.  This
+ * (x86_sorted_cap_flags[]) is an array of array indexes
+ * *referring* to x86_cap_flags[] entries.  It is sorted
+ * to make it quick to print a sorted list of cpu flags in
+ * /proc/cpuinfo.
+ */
+static unsigned short x86_sorted_cap_flags[X86_NR_CAPS] = { -1, };
+static int x86_cmp_cap(const void *a_ptr, const void *b_ptr)
+{
+	unsigned short a = *(unsigned short *)a_ptr;
+	unsigned short b = *(unsigned short *)b_ptr;
+
+	/* Don't need to swap equal entries (presumably NULLs) */
+	if (x86_cap_flags[a] == x86_cap_flags[b])
+		return 0;
+	/* Put NULL elements at the end: */
+	if (x86_cap_flags[a] == NULL)
+		return -1;
+	if (x86_cap_flags[b] == NULL)
+		return 1;
+
+	return strcmp(x86_cap_flags[a], x86_cap_flags[b]);
+}
+
+static void x86_sort_cap_flags(void)
+{
+	static DEFINE_SPINLOCK(lock);
+	int i;
+
+	/*
+	 * It's possible that multiple threads could race
+	 * to here and both sort the list.  The lock keeps
+	 * them from trying to sort concurrently.
+	 */
+	spin_lock(&lock);
+
+	/* Initialize the list with 0->i, removing the -1's: */
+	for (i = 0; i < X86_NR_CAPS; i++)
+		x86_sorted_cap_flags[i] = i;
+
+	sort(x86_sorted_cap_flags, X86_NR_CAPS,
+	     sizeof(x86_sorted_cap_flags[0]),
+	     x86_cmp_cap, NULL);
+
+	spin_unlock(&lock);
+}
+
+static void show_cpuinfo_flags(struct seq_file *m, struct cpuinfo_x86 *c)
+{
+	int i;
+
+	if (x86_sorted_cap_flags[0] == (unsigned short)-1)
+		x86_sort_cap_flags();
+
+	seq_puts(m, "flags\t\t:");
+
+	for (i = 0; i < X86_NR_CAPS; i++) {
+		/*
+		 * Go through the flag list in alphabetical
+		 * order to make reading this field easier.
+		 */
+		int cap = x86_sorted_cap_flags[i];
+
+		if (cpu_has(c, cap) && x86_cap_flags[cap] != NULL)
+			seq_printf(m, " %s", x86_cap_flags[cap]);
+	}
+}
+
 static int show_cpuinfo(struct seq_file *m, void *v)
 {
 	struct cpuinfo_x86 *c = v;

@@ -96,15 +168,11 @@ static int show_cpuinfo(struct seq_file
 
 	show_cpuinfo_core(m, c, cpu);
 	show_cpuinfo_misc(m, c);
-
-	seq_puts(m, "flags\t\t:");
-	for (i = 0; i < 32*NCAPINTS; i++)
-		if (cpu_has(c, i) && x86_cap_flags[i] != NULL)
-			seq_printf(m, " %s", x86_cap_flags[i]);
+	show_cpuinfo_flags(m, c);
 
 	seq_puts(m, "\nbugs\t\t:");
 	for (i = 0; i < 32*NBUGINTS; i++) {
-		unsigned int bug_bit = 32*NCAPINTS + i;
+		unsigned int bug_bit = x86_NR_CAPS + i;
 
 		if (cpu_has_bug(c, bug_bit) && x86_bug_flags[i])
 			seq_printf(m, " %s", x86_bug_flags[i]);

Kirill A. Shutemov

2018-12-20 12:02 UTC #1 lore raw

Quoted 33 lines · Dave Hansen

From: Dave Hansen <[email protected]>
I frequently find myself contemplating my life choices as I try to find 3-character entries in the 1,000-character, unsorted "flags:" field of /proc/cpuinfo.
Sort that field, giving me hours back in my day.
This eats up ~1200 bytes (NCAPINTS*2*32) of space for the sorted array. I used an 'unsigned short' to use 1/4 of the space on 64-bit that would have been needed had pointers been used in the array.
An alternatve, requiring no array, would be to do the sort at runtime, but it seems ridiculous for a 500-cpu system to do 500 sorts for each 'cat /proc/cpuinfo'.
Another would be to just cache the *string* that results from this, which would be even faster at runtime because it could do a single seq_printf() and would consume less space. But, that would require a bit more infrastructure to make sure that the produced string never changed and was consistent across all CPUs, unless we want to store a string per 'struct cpuinfo_x86'.
Signed-off-by Dave Hansen <[email protected]>
Cc [email protected]
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Jia Zhang <[email protected]>
Cc "Gustavo A. R. Silva" <[email protected]>
Cc [email protected]

Quoted 1 line

---

Below is my attempt on doing the same. The key difference is that the sorted array is generated at compile-time by mkcapflags.sh.

The patch also sorts bugs.

arch/x86/include/asm/cpufeature.h +2 -0

index 7d442722ef24..37cc8ccaec8f 100644
@@ -34,6 +34,7 @@ enum cpuid_leafs
 
 #ifdef CONFIG_X86_FEATURE_NAMES
 extern const char * const x86_cap_flags[NCAPINTS*32];
+extern const unsigned short x86_sorted_cap_flags[];
 extern const char * const x86_power_flags[32];
 #define X86_CAP_FMT "%s"
 #define x86_cap_flag(flag) x86_cap_flags[flag]

@@ -47,6 +48,7 @@ extern const char * const x86_power_flags[32];
  * X86_BUG_<name> - NCAPINTS*32.
  */
 extern const char * const x86_bug_flags[NBUGINTS*32];
+extern const unsigned short x86_sorted_bug_flags[];
 
 #define test_cpu_cap(c, bit)						\
 	 test_bit(bit, (unsigned long *)((c)->x86_capability))

arch/x86/kernel/cpu/mkcapflags.sh +55 ~~-19~~

99 lines not on this page · raw message

arch/x86/kernel/cpu/proc.c +47 ~~-13~~

72 lines not on this page · raw message

Signature · 1 line

 Kirill A. Shutemov

Borislav Petkov

maintainer

2018-12-20 16:04 UTC #2 lore raw

Quoted 2 lines · Kirill A. Shutemov

Below is my attempt on doing the same. The key difference is that the sorted array is generated at compile-time by mkcapflags.sh.

Let's just drop this silliness and use good old grep, ok?

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Kirill A. Shutemov

2018-12-21 12:40 UTC #3 lore raw

Quoted 5 lines · Borislav Petkov

Quoted 2 lines · Kirill A. Shutemov
Below is my attempt on doing the same. The key difference is that the sorted array is generated at compile-time by mkcapflags.sh.
Let's just drop this silliness and use good old grep, ok?

Okay. :/

But I don't see an improvement in readability of data presented to user as a silly idea.

Signature · 1 line

 Kirill A. Shutemov

Borislav Petkov

maintainer

2018-12-21 13:04 UTC #4 lore raw

Quoted 2 lines · Kirill A. Shutemov

But I don't see an improvement in readability of data presented to user as a silly idea.

Improving readability is not a silly idea and I never said that. Rather, the cost of what you're trying to accomplish, needs to be weighed.

The final goal of this is, AFAIU, finding whether a feature flag is there or not and you can use grep for that now, on *any* kernel.

And if you need the feature flags sorted, you can do that too:

$ grep -m 1 flags /proc/cpuinfo | tr " " "\n" | sort | xargs

and there probably is even a simpler way to do that.

Or add a shell alias for that or a small script or ...

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Kirill A. Shutemov

2018-12-21 13:19 UTC #5 lore raw

Quoted 10 lines · Borislav Petkov

Quoted 2 lines · Kirill A. Shutemov
But I don't see an improvement in readability of data presented to user as a silly idea.
Improving readability is not a silly idea and I never said that. Rather, the cost of what you're trying to accomplish, needs to be weighed.
The final goal of this is, AFAIU, finding whether a feature flag is there or not and you can use grep for that now, on *any* kernel.

Quoted 7 lines

And if you need the feature flags sorted, you can do that too:
$ grep -m 1 flags /proc/cpuinfo | tr " " "\n" | sort | xargs
and there probably is even a simpler way to do that.
Or add a shell alias for that or a small script or ...

That's very valid point.

Dave's patch made me recall few cases were I wanted to check a presence of a CPU flag with very limited userspace: shell in initrd with lean busybox build (no grep around). Eyeballing the flag took frustratingly long.

Yes, all this can be addressed in userspace one way or another. But it doesn't take much for kernel to present the data in a more reasonable way too...

Signature · 1 line

 Kirill A. Shutemov

Borislav Petkov

maintainer

2018-12-21 13:26 UTC #6 lore raw

Quoted 4 lines · Kirill A. Shutemov

Dave's patch made me recall few cases were I wanted to check a presence of a CPU flag with very limited userspace: shell in initrd with lean busybox build (no grep around). Eyeballing the flag took frustratingly long.

Easy:

less (or more) /proc/cpuinfo

and search there with the '/' command.

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Dave Hansen

2018-12-21 15:19 UTC #7 lore raw

In reply to Borislav Petkov

Quoted 6 lines · Borislav Petkov

$ grep -m 1 flags /proc/cpuinfo | tr " " "\n" | sort | xargs
and there probably is even a simpler way to do that.
Or add a shell alias for that or a small script or ...

I don't always look at these through the shell. I got a screenshot the other day, or someone pasted the flags into a spreadsheet.

/proc/cpuinfo is supposed to be human-readable. But, the flags field, as it stands, is not. You've further proved my point by having to machine post-process it to make it usable by humans. :)

Borislav Petkov

maintainer

2018-12-21 16:13 UTC #8 lore raw

Quoted 3 lines · Dave Hansen

/proc/cpuinfo is supposed to be human-readable. But, the flags field, as it stands, is not. You've further proved my point by having to machine post-process it to make it usable by humans. :)

No, it is usable now as it is. *You* claimed it isn't and I showed you that you can sort this thing without adding code to the kernel, so that you can have it sorted too, without the need to add more useless code to the kernel.

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Kirill A. Shutemov

2018-12-20 12:07 UTC #9 lore raw

In reply to Dave Hansen

Quoted 5 lines · Dave Hansen

arch/x86/kernel/cpu/proc.c renamed from arch/x86/kernel/cpu/proc.c~x86-sorted-flags +0 -1
@@ -96,15 +168,11 @@ static int show_cpuinfo(struct seq_file
 
 	show_cpuinfo_core(m, c, cpu);
 	show_cpuinfo_misc(m, c);
-

arch/x86/kernel/cpu/proc.c static int show_cpuinfo(struct seq_file · line 175

Quoted 10 lines

arch/x86/kernel/cpu/proc.c renamed from arch/x86/kernel/cpu/proc.c~x86-sorted-flags +2 -5

-	seq_puts(m, "flags\t\t:");
-	for (i = 0; i < 32*NCAPINTS; i++)
-		if (cpu_has(c, i) && x86_cap_flags[i] != NULL)
-			seq_printf(m, " %s", x86_cap_flags[i]);
+	show_cpuinfo_flags(m, c);
 
 	seq_puts(m, "\nbugs\t\t:");
 	for (i = 0; i < 32*NBUGINTS; i++) {
-		unsigned int bug_bit = 32*NCAPINTS + i;
+		unsigned int bug_bit = x86_NR_CAPS + i;

s/x86_NR_CAPS/X86_NR_CAPS/

Quoted 4 lines

arch/x86/kernel/cpu/proc.c renamed from arch/x86/kernel/cpu/proc.c~x86-sorted-flags +0 -0
 		if (cpu_has_bug(c, bug_bit) && x86_bug_flags[i])
 			seq_printf(m, " %s", x86_bug_flags[i]);
_

Signature · 1 line

 Kirill A. Shutemov

kbuild test robot

build bot

2018-12-21 09:37 UTC #10 lore raw

In reply to Dave Hansen

I love your patch! Yet something to improve:

Hi Dave,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc7 next-20181220]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Dave-Hansen/x86-cpu-sort-cpuinfo-flags/20181221-171144
config: x86_64-randconfig-x013-201850 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All errors (new ones prefixed by >>):

arch/x86//kernel/cpu/proc.c: In function 'show_cpuinfo':

>> arch/x86//kernel/cpu/proc.c:175:26: error: 'x86_NR_CAPS' undeclared (first use in this function); did you mean 'X86_NR_CAPS'?
      unsigned int bug_bit = x86_NR_CAPS + i;
                             ^~~~~~~~~~~
                             X86_NR_CAPS

arch/x86//kernel/cpu/proc.c:175:26: note: each undeclared identifier is reported only once for each function it appears in

vim +175 arch/x86//kernel/cpu/proc.c

128
129 static int show_cpuinfo(struct seq_file *m, void *v)
130 {
131 struct cpuinfo_x86 *c = v;
132 unsigned int cpu;
133 int i;
134
135 cpu = c->cpu_index;
136 seq_printf(m, "processor\t: %u\n"

   137			   "vendor_id\t: %s\n"
   138			   "cpu family\t: %d\n"
   139			   "model\t\t: %u\n"
   140			   "model name\t: %s\n",
   141			   cpu,
   142			   c->x86_vendor_id[0] ? c->x86_vendor_id : "unknown",
   143			   c->x86,
   144			   c->x86_model,
   145			   c->x86_model_id[0] ? c->x86_model_id : "unknown");
   146	
   147		if (c->x86_stepping || c->cpuid_level >= 0)
   148			seq_printf(m, "stepping\t: %d\n", c->x86_stepping);
   149		else
   150			seq_puts(m, "stepping\t: unknown\n");
   151		if (c->microcode)
   152			seq_printf(m, "microcode\t: 0x%x\n", c->microcode);
   153	
   154		if (cpu_has(c, X86_FEATURE_TSC)) {
   155			unsigned int freq = aperfmperf_get_khz(cpu);
   156	
   157			if (!freq)
   158				freq = cpufreq_quick_get(cpu);
   159			if (!freq)
   160				freq = cpu_khz;
   161			seq_printf(m, "cpu MHz\t\t: %u.%03u\n",
   162				   freq / 1000, (freq % 1000));
   163		}
   164	
   165		/* Cache size */
   166		if (c->x86_cache_size)
   167			seq_printf(m, "cache size\t: %u KB\n", c->x86_cache_size);
   168	
   169		show_cpuinfo_core(m, c, cpu);
   170		show_cpuinfo_misc(m, c);
   171		show_cpuinfo_flags(m, c);
   172	
   173		seq_puts(m, "\nbugs\t\t:");
   174		for (i = 0; i < 32*NBUGINTS; i++) {
 > 175			unsigned int bug_bit = x86_NR_CAPS + i;

176
177 if (cpu_has_bug(c, bug_bit) && x86_bug_flags[i])
178 seq_printf(m, " %s", x86_bug_flags[i]);
179 }
180
181 seq_printf(m, "\nbogomips\t: %lu.%02lu\n",
182 c->loops_per_jiffy/(500000/HZ),
183 (c->loops_per_jiffy/(5000/HZ)) % 100);
184

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

kbuild test robot

build bot

2018-12-22 17:39 UTC #11 lore raw

In reply to Dave Hansen

I love your patch! Yet something to improve:

Hi Dave,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc7 next-20181221]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Dave-Hansen/x86-cpu-sort-cpuinfo-flags/20181221-171144
config: x86_64-randconfig-s2-12221353 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All errors (new ones prefixed by >>):

arch/x86/kernel/cpu/proc.c: In function 'show_cpuinfo':

>> arch/x86/kernel/cpu/proc.c:175:26: error: 'x86_NR_CAPS' undeclared (first use in this function)
      unsigned int bug_bit = x86_NR_CAPS + i;
                             ^~~~~~~~~~~

arch/x86/kernel/cpu/proc.c:175:26: note: each undeclared identifier is reported only once for each function it appears in

vim +/x86_NR_CAPS +175 arch/x86/kernel/cpu/proc.c

   137			   "vendor_id\t: %s\n"
   138			   "cpu family\t: %d\n"
   139			   "model\t\t: %u\n"
   140			   "model name\t: %s\n",
   141			   cpu,
   142			   c->x86_vendor_id[0] ? c->x86_vendor_id : "unknown",
   143			   c->x86,
   144			   c->x86_model,
   145			   c->x86_model_id[0] ? c->x86_model_id : "unknown");
   146	
   147		if (c->x86_stepping || c->cpuid_level >= 0)
   148			seq_printf(m, "stepping\t: %d\n", c->x86_stepping);
   149		else
   150			seq_puts(m, "stepping\t: unknown\n");
   151		if (c->microcode)
   152			seq_printf(m, "microcode\t: 0x%x\n", c->microcode);
   153	
   154		if (cpu_has(c, X86_FEATURE_TSC)) {
   155			unsigned int freq = aperfmperf_get_khz(cpu);
   156	
   157			if (!freq)
   158				freq = cpufreq_quick_get(cpu);
   159			if (!freq)
   160				freq = cpu_khz;
   161			seq_printf(m, "cpu MHz\t\t: %u.%03u\n",
   162				   freq / 1000, (freq % 1000));
   163		}
   164	
   165		/* Cache size */
   166		if (c->x86_cache_size)
   167			seq_printf(m, "cache size\t: %u KB\n", c->x86_cache_size);
   168	
   169		show_cpuinfo_core(m, c, cpu);
   170		show_cpuinfo_misc(m, c);
   171		show_cpuinfo_flags(m, c);
   172	
   173		seq_puts(m, "\nbugs\t\t:");
   174		for (i = 0; i < 32*NBUGINTS; i++) {
 > 175			unsigned int bug_bit = x86_NR_CAPS + i;

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation