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 <[email protected]>
---
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;
}
--
On Fri, Mar 28, 2008 at 12:16 AM, Mike Travis <[email protected]> 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 <[email protected]>
> ---
> 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
Bert Wesarg wrote:
> On Fri, Mar 28, 2008 at 12:16 AM, Mike Travis <[email protected]> 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 <[email protected]>
>> ---
>> 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
On Fri, Mar 28, 2008 at 3:40 PM, Mike Travis <[email protected]> wrote:
>
> Bert Wesarg wrote:
> > On Fri, Mar 28, 2008 at 12:16 AM, Mike Travis <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
>
Bert Wesarg wrote:
> On Fri, Mar 28, 2008 at 3:40 PM, Mike Travis <[email protected]> wrote:
>> Bert Wesarg wrote:
>> > On Fri, Mar 28, 2008 at 12:16 AM, Mike Travis <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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
On Fri, Mar 28, 2008 at 7:19 PM, Mike Travis <[email protected]> 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
>
Bert Wesarg wrote:
> On Fri, Mar 28, 2008 at 7:19 PM, Mike Travis <[email protected]> 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
On Mon, Mar 31, 2008 at 6:35 PM, Mike Travis <[email protected]> wrote:
> Bert Wesarg wrote:
> > On Fri, Mar 28, 2008 at 7:19 PM, Mike Travis <[email protected]> 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
>
>> 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 <[email protected]> 1.940.382.4214
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 <[email protected]> 1.940.382.4214