[PATCH 0/2] x86: add functions in preparation of cpumask changes

[PATCH 0/2] x86: add functions in preparation of cpumask changes

[Mike Travis]

Some more small changes for x86 in preparation of larger cpumask changes.

    * Introduce the cpumask_of_cpu_map via Kconfig option

    * Modify show_shared_cpu_map in intel_cacheinfo.c to use
      cpulist_scnprintf() instead of cpumask_scnprintf().  This
      eliminates the need for the new function cpumask_scnprintf_len().

Based on:
	git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
	git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git

Signed-off-by: Mike Travis <travis@sgi.com>

-- 

[PATCH 1/2] x86: Convert cpumask_of_cpu macro to allocated array

[Mike Travis]

[-- Attachment #1: cpumask_of_cpu --]
[-- Type: text/plain, Size: 3482 bytes --]

Here is a simple patch to use an allocated array of cpumasks to
represent cpumask_of_cpu() instead of constructing one on the stack.
It's based on the Kconfig option "HAVE_CPUMASK_OF_CPU_MAP" which is
currently only set for x86_64 SMP.

Note that the existing cpumask_of_cpu() macro is also changed to
produce an lvalue so a pointer to it can be used.

Based on:
	git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
	git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git

Signed-off-by: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Mike Travis <travis@sgi.com>
---
 arch/x86/Kconfig        |    3 +++
 arch/x86/kernel/setup.c |   39 +++++++++++++++++++++++++++++++++++++++
 include/linux/cpumask.h |   12 +++++++++---
 3 files changed, 51 insertions(+), 3 deletions(-)

--- linux.trees.git.orig/arch/x86/Kconfig
+++ linux.trees.git/arch/x86/Kconfig
@@ -123,6 +123,9 @@ config ARCH_HAS_CPU_RELAX
 config HAVE_SETUP_PER_CPU_AREA
 	def_bool X86_64 || (X86_SMP && !X86_VOYAGER)
 
+config HAVE_CPUMASK_OF_CPU_MAP
+	def_bool X86_64_SMP
+
 config ARCH_HIBERNATION_POSSIBLE
 	def_bool y
 	depends on !SMP || !X86_VOYAGER
--- linux.trees.git.orig/arch/x86/kernel/setup.c
+++ linux.trees.git/arch/x86/kernel/setup.c
@@ -10,6 +10,21 @@
 #include <asm/setup.h>
 #include <asm/topology.h>
 
+#ifdef CONFIG_SMP
+/* setup nr_cpu_ids early */
+static void __init setup_nr_cpu_ids(void)
+{
+	int cpu, highest_cpu = 0;
+
+	for_each_possible_cpu(cpu)
+		highest_cpu = cpu;
+
+	nr_cpu_ids = highest_cpu + 1;
+}
+#else
+static inline void setup_nr_cpu_ids(void) { }
+#endif
+
 #if defined(CONFIG_HAVE_SETUP_PER_CPU_AREA) && defined(CONFIG_SMP)
 /*
  * Copy data used in early init routines from the initial arrays to the
@@ -38,6 +53,24 @@ static void __init setup_per_cpu_maps(vo
 #endif
 }
 
+#ifdef CONFIG_HAVE_CPUMASK_OF_CPU_MAP
+cpumask_t *cpumask_of_cpu_map __read_mostly;
+EXPORT_SYMBOL(cpumask_of_cpu_map);
+
+/* requires nr_cpu_ids to be initialized */
+static void __init setup_cpumask_of_cpu(void)
+{
+	int i;
+
+	/* alloc_bootmem zeroes memory */
+	cpumask_of_cpu_map = alloc_bootmem_low(sizeof(cpumask_t) * nr_cpu_ids);
+	for (i = 0; i < nr_cpu_ids; i++)
+		cpu_set(i, cpumask_of_cpu_map[i]);
+}
+#else
+static inline void setup_cpumask_of_cpu(void) { }
+#endif
+
 #ifdef CONFIG_X86_32
 /*
  * Great future not-so-futuristic plan: make i386 and x86_64 do it
@@ -90,8 +123,14 @@ void __init setup_per_cpu_areas(void)
 		memcpy(ptr, __per_cpu_start, __per_cpu_end - __per_cpu_start);
 	}
 
+	/* Setup nr_cpu_ids */
+	setup_nr_cpu_ids();
+
 	/* Setup percpu data maps */
 	setup_per_cpu_maps();
+
+	/* Setup cpumask_of_cpu map */
+	setup_cpumask_of_cpu();
 }
 
 #endif
--- linux.trees.git.orig/include/linux/cpumask.h
+++ linux.trees.git/include/linux/cpumask.h
@@ -222,8 +222,13 @@ int __next_cpu(int n, const cpumask_t *s
 #define next_cpu(n, src)	({ (void)(src); 1; })
 #endif
 
+#ifdef CONFIG_HAVE_CPUMASK_OF_CPU_MAP
+extern cpumask_t *cpumask_of_cpu_map;
+#define cpumask_of_cpu(cpu)    (cpumask_of_cpu_map[cpu])
+
+#else
 #define cpumask_of_cpu(cpu)						\
-({									\
+(*({									\
 	typeof(_unused_cpumask_arg_) m;					\
 	if (sizeof(m) == sizeof(unsigned long)) {			\
 		m.bits[0] = 1UL<<(cpu);					\
@@ -231,8 +236,9 @@ int __next_cpu(int n, const cpumask_t *s
 		cpus_clear(m);						\
 		cpu_set((cpu), m);					\
 	}								\
-	m;								\
-})
+	&m;								\
+}))
+#endif
 
 #define CPU_MASK_LAST_WORD BITMAP_LAST_WORD_MASK(NR_CPUS)
 

-- 

[PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Mike Travis]

[-- Attachment #1: mod-intel_cacheinfo --]
[-- Type: text/plain, Size: 1805 bytes --]

Used cpulist_scnprintf to print cpus on a leaf instead of requiring
a new "cpumask_scnprintf_len" function to determine the size of
the temporary buffer.  cpulist_scnprintf can be used to print directly
to the output buffer, eliminating the need for the temporary buffer.

Based on:
	git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
	git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git

Signed-off-by: Mike Travis <travis@sgi.com>
---
 arch/x86/kernel/cpu/intel_cacheinfo.c |   25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

--- linux.trees.git.orig/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ linux.trees.git/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -593,14 +593,23 @@ static ssize_t show_size(struct _cpuid4_
 
 static ssize_t show_shared_cpu_map(struct _cpuid4_info *this_leaf, char *buf)
 {
-	int n = 0;
-	int len = cpumask_scnprintf_len(nr_cpu_ids);
-	char *mask_str = kmalloc(len, GFP_KERNEL);
-
-	if (mask_str) {
-		cpumask_scnprintf(mask_str, len, this_leaf->shared_cpu_map);
-		n = sprintf(buf, "%s\n", mask_str);
-		kfree(mask_str);
+	/*
+	 * cpulist_scnprintf() has the advantage of compressing
+	 * consecutive cpu numbers into a single range which seems
+	 * appropriate for cpus on a leaf.  This will change what is
+	 * output so scripts that process the output will have to change.
+	 * The good news is that the output format is compatible
+	 * with cpulist_parse() [bitmap_parselist()].
+	 *
+	 * Have to guess at output buffer size... 128 seems reasonable
+	 * to represent all cpus on a leaf in the worst case, like
+	 * if all cpus are non-consecutive and large numbers.
+	 */
+	int n = cpulist_scnprintf(buf, 128, this_leaf->shared_cpu_map);
+
+	if (n > 0) {
+		sprintf(buf+n, "\n");
+		n++;
 	}
 	return n;
 }

-- 

Re: [PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Bert Wesarg]

On Fri, Mar 28, 2008 at 12:16 AM, Mike Travis <travis@sgi.com> wrote:
> Used cpulist_scnprintf to print cpus on a leaf instead of requiring
>  a new "cpumask_scnprintf_len" function to determine the size of
>  the temporary buffer.  cpulist_scnprintf can be used to print directly
>  to the output buffer, eliminating the need for the temporary buffer.
>
>  Based on:
>         git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
>         git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git
>
>  Signed-off-by: Mike Travis <travis@sgi.com>
>  ---
>   arch/x86/kernel/cpu/intel_cacheinfo.c |   25 +++++++++++++++++--------
>   1 file changed, 17 insertions(+), 8 deletions(-)
>
>  --- linux.trees.git.orig/arch/x86/kernel/cpu/intel_cacheinfo.c
>  +++ linux.trees.git/arch/x86/kernel/cpu/intel_cacheinfo.c
>  @@ -593,14 +593,23 @@ static ssize_t show_size(struct _cpuid4_
>
>   static ssize_t show_shared_cpu_map(struct _cpuid4_info *this_leaf, char *buf)
>   {
>  -       int n = 0;
>  -       int len = cpumask_scnprintf_len(nr_cpu_ids);
>  -       char *mask_str = kmalloc(len, GFP_KERNEL);
>  -
>  -       if (mask_str) {
>  -               cpumask_scnprintf(mask_str, len, this_leaf->shared_cpu_map);
>  -               n = sprintf(buf, "%s\n", mask_str);
>  -               kfree(mask_str);
>  +       /*
>  +        * cpulist_scnprintf() has the advantage of compressing
>  +        * consecutive cpu numbers into a single range which seems
>  +        * appropriate for cpus on a leaf.  This will change what is
>  +        * output so scripts that process the output will have to change.
So this breaks user space?

Bert

Re: [PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Mike Travis]

Bert Wesarg wrote:
> On Fri, Mar 28, 2008 at 12:16 AM, Mike Travis <travis@sgi.com> wrote:
>> Used cpulist_scnprintf to print cpus on a leaf instead of requiring
>>  a new "cpumask_scnprintf_len" function to determine the size of
>>  the temporary buffer.  cpulist_scnprintf can be used to print directly
>>  to the output buffer, eliminating the need for the temporary buffer.
>>
>>  Based on:
>>         git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
>>         git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git
>>
>>  Signed-off-by: Mike Travis <travis@sgi.com>
>>  ---
>>   arch/x86/kernel/cpu/intel_cacheinfo.c |   25 +++++++++++++++++--------
>>   1 file changed, 17 insertions(+), 8 deletions(-)
>>
>>  --- linux.trees.git.orig/arch/x86/kernel/cpu/intel_cacheinfo.c
>>  +++ linux.trees.git/arch/x86/kernel/cpu/intel_cacheinfo.c
>>  @@ -593,14 +593,23 @@ static ssize_t show_size(struct _cpuid4_
>>
>>   static ssize_t show_shared_cpu_map(struct _cpuid4_info *this_leaf, char *buf)
>>   {
>>  -       int n = 0;
>>  -       int len = cpumask_scnprintf_len(nr_cpu_ids);
>>  -       char *mask_str = kmalloc(len, GFP_KERNEL);
>>  -
>>  -       if (mask_str) {
>>  -               cpumask_scnprintf(mask_str, len, this_leaf->shared_cpu_map);
>>  -               n = sprintf(buf, "%s\n", mask_str);
>>  -               kfree(mask_str);
>>  +       /*
>>  +        * cpulist_scnprintf() has the advantage of compressing
>>  +        * consecutive cpu numbers into a single range which seems
>>  +        * appropriate for cpus on a leaf.  This will change what is
>>  +        * output so scripts that process the output will have to change.
> So this breaks user space?
> 
> Bert

Potentially, yes.  But I suspect with 4096 cpus, user scripts will have
to change anyways.  Currently it is represented as sets of 32 bit mask
outputs with comma separators, so 1152 characters would be output.

Is there a special notice I should provide to announce this change?

(And this output does conform with other syntax for printing and parsing
strings of bits.)

Thanks,
Mike

Re: [PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Bert Wesarg]

On Fri, Mar 28, 2008 at 3:40 PM, Mike Travis <travis@sgi.com> wrote:
>
> Bert Wesarg wrote:
>  > On Fri, Mar 28, 2008 at 12:16 AM, Mike Travis <travis@sgi.com> wrote:
>  >> Used cpulist_scnprintf to print cpus on a leaf instead of requiring
>  >>  a new "cpumask_scnprintf_len" function to determine the size of
>  >>  the temporary buffer.  cpulist_scnprintf can be used to print directly
>  >>  to the output buffer, eliminating the need for the temporary buffer.
>  >>
>  >>  Based on:
>  >>         git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
>  >>         git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git
>  >>
>  >>  Signed-off-by: Mike Travis <travis@sgi.com>
>  >>  ---
>  >>   arch/x86/kernel/cpu/intel_cacheinfo.c |   25 +++++++++++++++++--------
>  >>   1 file changed, 17 insertions(+), 8 deletions(-)
>  >>
>  >>  --- linux.trees.git.orig/arch/x86/kernel/cpu/intel_cacheinfo.c
>  >>  +++ linux.trees.git/arch/x86/kernel/cpu/intel_cacheinfo.c
>  >>  @@ -593,14 +593,23 @@ static ssize_t show_size(struct _cpuid4_
>  >>
>  >>   static ssize_t show_shared_cpu_map(struct _cpuid4_info *this_leaf, char *buf)
>  >>   {
>  >>  -       int n = 0;
>  >>  -       int len = cpumask_scnprintf_len(nr_cpu_ids);
>  >>  -       char *mask_str = kmalloc(len, GFP_KERNEL);
>  >>  -
>  >>  -       if (mask_str) {
>  >>  -               cpumask_scnprintf(mask_str, len, this_leaf->shared_cpu_map);
>  >>  -               n = sprintf(buf, "%s\n", mask_str);
>  >>  -               kfree(mask_str);
>  >>  +       /*
>  >>  +        * cpulist_scnprintf() has the advantage of compressing
>  >>  +        * consecutive cpu numbers into a single range which seems
>  >>  +        * appropriate for cpus on a leaf.  This will change what is
>  >>  +        * output so scripts that process the output will have to change.
>  > So this breaks user space?
>  >
>  > Bert
>
>  Potentially, yes.  But I suspect with 4096 cpus, user scripts will have
>  to change anyways.  Currently it is represented as sets of 32 bit mask
>  outputs with comma separators, so 1152 characters would be output.
But you can declare it as a programming error on user space side. If
you change the format, the brown-paper-bag is yours.

>
>  Is there a special notice I should provide to announce this change?
I hope so. At least lwn.net has an API changes site:

http://lwn.net/Articles/2.6-kernel-api/

I also looked into MAINTAINERS, but it seems there is no official API
'maintainer'.

>
>  (And this output does conform with other syntax for printing and parsing
>  strings of bits.)
Aren't the most cpumaps (like cpu/cpu*/topology/*_siblings or
node/node*/cpumap) bitmasks?

Bert
>
>  Thanks,
>  Mike
>

Re: [PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Mike Travis]

Bert Wesarg wrote:
> On Fri, Mar 28, 2008 at 3:40 PM, Mike Travis <travis@sgi.com> wrote:
>> Bert Wesarg wrote:
>>  > On Fri, Mar 28, 2008 at 12:16 AM, Mike Travis <travis@sgi.com> wrote:
>>  >> Used cpulist_scnprintf to print cpus on a leaf instead of requiring
>>  >>  a new "cpumask_scnprintf_len" function to determine the size of
>>  >>  the temporary buffer.  cpulist_scnprintf can be used to print directly
>>  >>  to the output buffer, eliminating the need for the temporary buffer.
>>  >>
>>  >>  Based on:
>>  >>         git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
>>  >>         git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git
>>  >>
>>  >>  Signed-off-by: Mike Travis <travis@sgi.com>
>>  >>  ---
>>  >>   arch/x86/kernel/cpu/intel_cacheinfo.c |   25 +++++++++++++++++--------
>>  >>   1 file changed, 17 insertions(+), 8 deletions(-)
>>  >>
>>  >>  --- linux.trees.git.orig/arch/x86/kernel/cpu/intel_cacheinfo.c
>>  >>  +++ linux.trees.git/arch/x86/kernel/cpu/intel_cacheinfo.c
>>  >>  @@ -593,14 +593,23 @@ static ssize_t show_size(struct _cpuid4_
>>  >>
>>  >>   static ssize_t show_shared_cpu_map(struct _cpuid4_info *this_leaf, char *buf)
>>  >>   {
>>  >>  -       int n = 0;
>>  >>  -       int len = cpumask_scnprintf_len(nr_cpu_ids);
>>  >>  -       char *mask_str = kmalloc(len, GFP_KERNEL);
>>  >>  -
>>  >>  -       if (mask_str) {
>>  >>  -               cpumask_scnprintf(mask_str, len, this_leaf->shared_cpu_map);
>>  >>  -               n = sprintf(buf, "%s\n", mask_str);
>>  >>  -               kfree(mask_str);
>>  >>  +       /*
>>  >>  +        * cpulist_scnprintf() has the advantage of compressing
>>  >>  +        * consecutive cpu numbers into a single range which seems
>>  >>  +        * appropriate for cpus on a leaf.  This will change what is
>>  >>  +        * output so scripts that process the output will have to change.
>>  > So this breaks user space?
>>  >
>>  > Bert
>>
>>  Potentially, yes.  But I suspect with 4096 cpus, user scripts will have
>>  to change anyways.  Currently it is represented as sets of 32 bit mask
>>  outputs with comma separators, so 1152 characters would be output.
> But you can declare it as a programming error on user space side. If
> you change the format, the brown-paper-bag is yours.
> 
>>  Is there a special notice I should provide to announce this change?
> I hope so. At least lwn.net has an API changes site:
> 
> http://lwn.net/Articles/2.6-kernel-api/

I did look at that site.   Besides being "kind of" out of date (last mod: 10/19/07),
it didn't appear to track changes in information displayed by proc/sysfs functions.

> 
> I also looked into MAINTAINERS, but it seems there is no official API
> 'maintainer'.
> 
>>  (And this output does conform with other syntax for printing and parsing
>>  strings of bits.)
> Aren't the most cpumaps (like cpu/cpu*/topology/*_siblings or
> node/node*/cpumap) bitmasks?

I did an informal survey and you are right, the majority of references do use
cpumask_scnprintf instead of cpulist_scnprintf.  Maybe the later function was
added later?

To me though, it would seem that:

240-255

is more readable than:

00000000,00000000,00000000,00000000,00000000,00000000,00000000,0000ffff

And as I mentioned, bitmask_parselist() [libbitmask(3)] does parse the output.

Thanks,
Mike

Re: [PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Bert Wesarg]

On Fri, Mar 28, 2008 at 7:19 PM, Mike Travis <travis@sgi.com> wrote:
>  > Aren't the most cpumaps (like cpu/cpu*/topology/*_siblings or
>  > node/node*/cpumap) bitmasks?
>
>  I did an informal survey and you are right, the majority of references do use
>  cpumask_scnprintf instead of cpulist_scnprintf.  Maybe the later function was
>  added later?
>
>  To me though, it would seem that:
>
>  240-255
>
>  is more readable than:
>
>  00000000,00000000,00000000,00000000,00000000,00000000,00000000,0000ffff
>
>  And as I mentioned, bitmask_parselist() [libbitmask(3)] does parse the output.
But libbitmask has a bitmask_parsehex() too. (but thanks for the
pointer to this code).

Anyway, your above example is wrong, the most significant bits comes first:

ffff0000,00000000,00000000,00000000,00000000,00000000,00000000,00000000

This makes it not more readable, but I think readability isn't in this
case of that much importance.

I further think, this problem could be easily solved, if NR_CPUS and
possibly your nr_cpus_ids is somehow exported to user space.

With this information, the user is not surprised to see more that 1024
bits (=CPU_SETSIZE, which is currently the glibc constant for the
sched_{set,get}affinity() API). Also the glibc has the new variable
cpu_set_t size API (since 2.7).

Bert
>
>  Thanks,
>  Mike
>

Re: [PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Mike Travis]

Bert Wesarg wrote:
> On Fri, Mar 28, 2008 at 7:19 PM, Mike Travis <travis@sgi.com> wrote:
>>  > Aren't the most cpumaps (like cpu/cpu*/topology/*_siblings or
>>  > node/node*/cpumap) bitmasks?
>>
>>  I did an informal survey and you are right, the majority of references do use
>>  cpumask_scnprintf instead of cpulist_scnprintf.  Maybe the later function was
>>  added later?
>>
>>  To me though, it would seem that:
>>
>>  240-255
>>
>>  is more readable than:
>>
>>  00000000,00000000,00000000,00000000,00000000,00000000,00000000,0000ffff
>>
>>  And as I mentioned, bitmask_parselist() [libbitmask(3)] does parse the output.
> But libbitmask has a bitmask_parsehex() too. (but thanks for the
> pointer to this code).
> 
> Anyway, your above example is wrong, the most significant bits comes first:
> 
> ffff0000,00000000,00000000,00000000,00000000,00000000,00000000,00000000
> 
> This makes it not more readable, but I think readability isn't in this
> case of that much importance.

The original problem was how to avoid allocating a large stack space to display
cpu ids.  By using cpulist_scnprintf, it accomplishes this without, what I think
is too much pain.  If it's really that much of a problem, I will rework this patch.
But the length of the line with 4096 cpus will be 1152 bytes  Is this really
better?

> 
> I further think, this problem could be easily solved, if NR_CPUS and
> possibly your nr_cpus_ids is somehow exported to user space.
> 
> With this information, the user is not surprised to see more that 1024
> bits (=CPU_SETSIZE, which is currently the glibc constant for the
> sched_{set,get}affinity() API). Also the glibc has the new variable
> cpu_set_t size API (since 2.7).

Yes, thanks.  That is being dealt with in another task.

Thanks,
Mike

Re: [PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Bert Wesarg]

On Mon, Mar 31, 2008 at 6:35 PM, Mike Travis <travis@sgi.com> wrote:
> Bert Wesarg wrote:
>  > On Fri, Mar 28, 2008 at 7:19 PM, Mike Travis <travis@sgi.com> wrote:
>  >>  > Aren't the most cpumaps (like cpu/cpu*/topology/*_siblings or
>  >>  > node/node*/cpumap) bitmasks?
>  >>
>  >>  I did an informal survey and you are right, the majority of references do use
>  >>  cpumask_scnprintf instead of cpulist_scnprintf.  Maybe the later function was
>  >>  added later?
>  >>
>  >>  To me though, it would seem that:
>  >>
>  >>  240-255
>  >>
>  >>  is more readable than:
>  >>
>  >>  00000000,00000000,00000000,00000000,00000000,00000000,00000000,0000ffff
>  >>
>  >>  And as I mentioned, bitmask_parselist() [libbitmask(3)] does parse the output.
>  > But libbitmask has a bitmask_parsehex() too. (but thanks for the
>  > pointer to this code).
>  >
>  > Anyway, your above example is wrong, the most significant bits comes first:
>  >
>  > ffff0000,00000000,00000000,00000000,00000000,00000000,00000000,00000000
>  >
>  > This makes it not more readable, but I think readability isn't in this
>  > case of that much importance.
>
>  The original problem was how to avoid allocating a large stack space to display
>  cpu ids.  By using cpulist_scnprintf, it accomplishes this without, what I think
>  is too much pain.  If it's really that much of a problem, I will rework this patch.
>  But the length of the line with 4096 cpus will be 1152 bytes  Is this really
>  better?
I ask myself, why is there a temporary buffer allocation in the first
place? In the end it is copied unbounded into the provided buf
argument. Sure your list is mostly shorter than a hex mask, but you
can also not be sure that it fit into the provided buffer. So you can
also use cpumask_scnprintf directly with the buf argument, and provide
a good known upper bound for the size (ie.
cpumask_scnprintf_len(nr_cpu_ids)).

>
>
>  >
>  > I further think, this problem could be easily solved, if NR_CPUS and
>  > possibly your nr_cpus_ids is somehow exported to user space.
>  >
>  > With this information, the user is not surprised to see more that 1024
>  > bits (=CPU_SETSIZE, which is currently the glibc constant for the
>  > sched_{set,get}affinity() API). Also the glibc has the new variable
>  > cpu_set_t size API (since 2.7).
>
>  Yes, thanks.  That is being dealt with in another task.
Can you keep me up to date. Thanks.

Bert
>
>  Thanks,
>  Mike
>

Re: [PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Paul Jackson]

Bert wrote:
> Sure your list is mostly shorter than a hex mask

"mostly" -- yup.

The masks always take 9 chars per 32 bits of the full mask size
(whether zeros or ones.)

The lists can take up to approx one to three chars per bit, for the
worst case lists of every other bit, depending on the log base 10 of
the highest included bit.

Consider for example:

  97,99,101,103,105,107,109,111,113,115,117,119,121,123,125,127

vs.

  AAAAAAAA,00000000,00000000,00000000

which, if I did my math right, are the same value.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214

Re: [PATCH 2/2] x86: modify show_shared_cpu_map in intel_cacheinfo

[Paul Jackson]

>>  I did an informal survey and you are right, the majority of references do use
>>  cpumask_scnprintf instead of cpulist_scnprintf.  Maybe the later function was
>>  added later?

My recollection is that I added cpulist_scnprintf later, yes.
Looking at my email archives, I see the mask versions mentioned
starting Feb 2004, and the list versions starting Aug 2004.

My rule of thumb has been to use the mask style (00000000,0000ffff)
for lower level interfaces, and the list style (0-15) for higher level
interfaces.

For long lists, the list style is easier for humans to read, but for
one word masks, the mask style can be easier to read for -some-
purposes and are more commonly used.

If you throw enough user level software at them, the lists are no more
or less difficult to form or parse.  Hand coded C parsers are probably
easier to write for the mask style, and might be closer to what low
level (closer to the hardware) programmers expect.

Certainly, a particular interface should not change once it goes public.

Once picked for a new interface, I don't recall ever seeing any significant
controversy over which one was picked.  So another of my rules of thumb
might apply here -- coders choice.  He who writes the code gets to make
the open choices.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.940.382.4214