[PATCH] - prof_cpu_mask problems

[PATCH] - prof_cpu_mask problems

[Jack Steiner]

I ran into several problems with the prof_cpu_mask & other
irq masks.

	- Displaying the profiling mask via "cat /proc/irq/prof_cpu_mask"
	  clears the mask.

	- the IRQ masks are displayed in an unusual format:
		# echo 12345678 >prof_cpu_mask
		# cat prof_cpu_mask
		5678123400000000



--- linux_base/arch/ia64/kernel/irq.c	Sun Nov 16 13:02:56 2003
+++ linux/arch/ia64/kernel/irq.c	Sun Nov 16 13:28:30 2003
@@ -974,18 +974,18 @@
 static int irq_affinity_read_proc (char *page, char **start, off_t off,
 			int count, int *eof, void *data)
 {
-	int k, len;
-	cpumask_t tmp = irq_affinity[(long)data];
+	cpumask_t mask, irqmask = irq_affinity[(long)data];
+	int i, j, k, len = 0;
 
 	if (count < HEX_DIGITS+1)
 		return -EINVAL;
 
-	len = 0;
-	for (k = 0; k < sizeof(cpumask_t)/sizeof(u16); ++k) {
-		int j = sprintf(page, "%04hx", (u16)cpus_coerce(tmp));
+	i = sizeof(cpumask_t)/sizeof(u16);
+	for (k = 0; k < i; ++k) {
+		cpus_shift_right(mask, irqmask, 16*(i-k-1));
+		j = sprintf(page, "%04hx", (u16)cpus_coerce(mask));
 		len += j;
 		page += j;
-		cpus_shift_right(tmp, tmp, 16);
 	}
 	len += sprintf(page, "\n");
 	return len;
@@ -1033,17 +1033,18 @@
 static int prof_cpu_mask_read_proc (char *page, char **start, off_t off,
 			int count, int *eof, void *data)
 {
-	cpumask_t *mask = (cpumask_t *)data;
-	int k, len = 0;
+	cpumask_t mask, profmask = *(cpumask_t *)data;
+	int i, j, k, len = 0;
 
 	if (count < HEX_DIGITS+1)
 		return -EINVAL;
 
-	for (k = 0; k < sizeof(cpumask_t)/sizeof(u16); ++k) {
-		int j = sprintf(page, "%04hx", (u16)cpus_coerce(*mask));
+	i = sizeof(cpumask_t)/sizeof(u16);
+	for (k = 0; k < i; ++k) {
+		cpus_shift_right(mask, profmask, 16*(i-k-1));
+		j = sprintf(page, "%04hx", (u16)cpus_coerce(mask));
 		len += j;
 		page += j;
-		cpus_shift_right(*mask, *mask, 16);
 	}
 	len += sprintf(page, "\n");
 	return len;
-- 
Thanks

Jack Steiner (steiner@sgi.com)          651-683-5302
Principal Engineer                      SGI - Silicon Graphics, Inc.

Re: [PATCH] - prof_cpu_mask problems

[Matthew Wilcox]

On Sun, Nov 16, 2003 at 01:30:43PM -0600, Jack Steiner wrote:
> 
> I ran into several problems with the prof_cpu_mask & other
> irq masks.

> -	len = 0;
> -	for (k = 0; k < sizeof(cpumask_t)/sizeof(u16); ++k) {
> -		int j = sprintf(page, "%04hx", (u16)cpus_coerce(tmp));
> +	i = sizeof(cpumask_t)/sizeof(u16);
> +	for (k = 0; k < i; ++k) {
> +		cpus_shift_right(mask, irqmask, 16*(i-k-1));
> +		j = sprintf(page, "%04hx", (u16)cpus_coerce(mask));
>  		len += j;
>  		page += j;
> -		cpus_shift_right(tmp, tmp, 16);

Why u16 rather than u32?  cpumask_t is guaranteed to be a multiple of
4, right?

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

RE: [PATCH] - prof_cpu_mask problems

[Luck, Tony]

> 	- the IRQ masks are displayed in an unusual format:
> 		# echo 12345678 >prof_cpu_mask
> 		# cat prof_cpu_mask
> 		5678123400000000

Although it's ugly ... that format seems to have been adopted in
generic code ... here's the code from drivers/base/node.c to
display the bitmap of cpus that belong to a node:

        for (k = 0; k < sizeof(cpumask_t)/sizeof(u16); ++k) {
                int j = sprintf(buf, "%04hx", (u16)cpus_coerce(tmp));
                len += j;
                buf += j;
                cpus_shift_right(tmp, tmp, 16);
        }

There should be some consistency in how bitmaps from cpumask_t
get displayed, but it would also be nice if the 'write' interfaces
to settable cpumasks used the same format that the 'read'
interfaces printed.  E.g. I might want to do:

 # cat /sys/devices/system/node/node0/cpumap > /proc/irq/prof_cpu_mask

-Tony

Re: [PATCH] - prof_cpu_mask problems

[Matthew Wilcox]

On Mon, Nov 17, 2003 at 09:16:01AM -0800, Luck, Tony wrote:
> Although it's ugly ... that format seems to have been adopted in
> generic code ... here's the code from drivers/base/node.c to
> display the bitmap of cpus that belong to a node:

William Irwin has a patch to fix this case.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] - prof_cpu_mask problems

[Paul Jackson]

Matthew wrote (on the linux-ia64 list):
> Why u16 rather than u32?

You'd probably have to ask whomever wrote that part in the first place.
I'd guess Jack was using u16 because it was already there.

My take - we should:
 1) change the loop to u32,
 2) add a separator character, say comma ',', between u32 words, and
 3) drop the zero %04x fill.
 
Then systems with NR_CPUS <= 32 would see a single hex number:
 "1"          if just cpu 0
 "80000000"   if just cpu 31
 
A full-up cpumask on a 512 CPU system would look like:

ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff,ffffffff

Typical systems with 4 or fewer CPUs would see masks ranging from
"1" to "f" (a single hex digit), and an empty mask of "0".

The user of a separator character becomes rather helpful for any
sort of human parsing on large systems, and is of no consequence
to those with 32 or fewer CPUs, which I am told is what most
poor deprived Linux programmers still have to cope with - sorry
<grin>.  Once a separator character is introduced, then one can
drop the zero-fill of each word, because one no longer needs
to explicitly display every digit to avoid ambiguous output.

For sparse masks on very large systems, these changes can be
a significant improvement, in my view.  An unbroken string of
128 hex digits is mind numbing.

Last night I posted the code snippet below on the
"format_cpumask()" thread of Bill Irwin's on lkml.  Bill, happy
to pass this tar baby onto me I suspect, replied "run with it".

I will try making the following into a real patch (just the
formatting stuff shown here, not the abstract data type,
for now).

The no-zero fill and the separator character are a tad different.

Speak up if you object ...

Special thanks to Christoph Hellwig for earlier comments (not
that he would necessarily approve of where I've taken this ...).

=
The following code provides the "," separated, no-zero fill,
flavor of this.  This comes from some work I am doing to provide
an abstract data type for a new nodemask_t type, that should
also work for cpumasks, with a nice little bit of code reduction
(but still nice inline assembly code in most cases).

The following "__mask_snprintf_len()" code would be called from
a macro, such as:

#define cpumask_snprintf(buf, buflen, cpumask) \
	__mask_snprintf_len(buf, buflen, cpumask_addr(cpumask), NR_CPUS/8)

where cpumask_addr(map) was one of:

	&(map)
    or
	(map).mask


int __mask_snprintf_len(char *buf, unsigned int buflen,
        unsigned int *maskp, unsigned int maskbytes)
{
        int i;
        int len = 0;
        unsigned int maskints = maskbytes/sizeof(unsigned int);
        char *sep;
        if (buflen < 2)
                return;
        if (maskints = 0) {
                strcpy(buf,"0");
                return 1;
        }
        buf[0] = 0;
        sep = "";
        for (i = maskints-1; i >= 0; i--) {
                int n = strlen(buf);
                len += snprintf(buf+n, buflen-n, "%s%x", sep, maskp[i]);
                sep = ",";
        }
        return len;
}


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

Re: [PATCH] - prof_cpu_mask problems

[William Lee Irwin III]

Matthew wrote (on the linux-ia64 list):
>> Why u16 rather than u32?

On Wed, Nov 19, 2003 at 01:45:30PM -0800, Paul Jackson wrote:
> You'd probably have to ask whomever wrote that part in the first place.
> I'd guess Jack was using u16 because it was already there.
> My take - we should:
>  1) change the loop to u32,
>  2) add a separator character, say comma ',', between u32 words, and
>  3) drop the zero %04x fill.

It was to silence a warning. You'll have write your own to replace
the fix now in -mm if this is what you really want.


-- wli

Re: [PATCH] - prof_cpu_mask problems

[Paul Jackson]

> It was to silence a warning.

ah - ok - thanks.


> You'll have write your own to replace
> the fix now in -mm if this is what you really want.

I'm doing that now; and un-duplicating about a dozen instances of the
binary->string conversion loops used when reading out masks, and almost
as many string->binary parse_hex_value() routines used when reading in
masks.  There will be a single routine for each, in a new lib file.

Can you tell me, Bill, how it is that having:

  #define HEX_DIGITS (2*sizeof(cpumask_t))

leaves room for the terminating nul-byte?  Seems to me that
a lot of hexnum[] arrays might be one byte short.

One incompatibility with existing code:

    If I proceed along my current path, cpumasks, such as for
    irq or smp_affinity, will no longer be displayed in /proc
    with leading zero padding.

    I expect that this could be a problem ...

Complaints invited.

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

Re: [PATCH] - prof_cpu_mask problems

[Paul Jackson]

> There will be a single routine for each, in a new lib file.

My apologies to Bill Irwin.  As I was well aware earlier today,
and had already spaced out, this removal of duplicate code is
already a feature of his latest -mm format_cpumask() patch.

I should not have spoken as if I was breaking new ground there.

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

Re: [PATCH] - prof_cpu_mask problems

[William Lee Irwin III]

At some point in the past, I wrote:
>> You'll have write your own to replace
>> the fix now in -mm if this is what you really want.

On Wed, Nov 19, 2003 at 06:17:03PM -0800, Paul Jackson wrote:
> I'm doing that now; and un-duplicating about a dozen instances of the
> binary->string conversion loops used when reading out masks, and almost
> as many string->binary parse_hex_value() routines used when reading in
> masks.  There will be a single routine for each, in a new lib file.

The format_cpumask() bits did that in the process of standardizing
and/or cleaning up the output. I stuffed it into the cpumask.h header,
but uninlining and/or grabbing a dedicated file is probably a decent
idea, since it may be too large for cpumask.h especially if the parsing
code gets into the picture.


On Wed, Nov 19, 2003 at 06:17:03PM -0800, Paul Jackson wrote:
> Can you tell me, Bill, how it is that having:
>   #define HEX_DIGITS (2*sizeof(cpumask_t))
> leaves room for the terminating nul-byte?  Seems to me that
> a lot of hexnum[] arrays might be one byte short.

I'm not convinced it does. If you could clean up correctness
issues you can find surrounding the parsing code, I'd be much obliged.
I got overextended trying to keep up with numerous things in and around
various architectures, and the irq affinity stuff didn't get heavily
reviewed, commented on, or tested.


On Wed, Nov 19, 2003 at 06:17:03PM -0800, Paul Jackson wrote:
> One incompatibility with existing code:
>     If I proceed along my current path, cpumasks, such as for
>     irq or smp_affinity, will no longer be displayed in /proc
>     with leading zero padding.
>     I expect that this could be a problem ...
> Complaints invited.

I don't see any problems with changing the format at this stage of
the game, even though it may be late in the release process. This is
relatively obscure stuff, and frankly, I suspect you (SGI) will be the
primary users of it (and the sole user for at least several months), so
whatever your favorite format is is pretty much how it should look.


-- wli

Re: [PATCH] - prof_cpu_mask problems

[Paul Jackson]

> Can you tell me, Bill, how it is that having:
> 
>   #define HEX_DIGITS (2*sizeof(cpumask_t))
> 
> leaves room for the terminating nul-byte?  Seems to me that
> a lot of hexnum[] arrays might be one byte short.

To answer my own question - on the output side (reads from /proc), the
code verifies that the user provided HEX_DIGITS+1 count bytes - good.

On the input side (writes into /proc), the code doesn't rely on a
nul-terminator, and ensures that only at most HEX_DIGITS worth are
copied into hexnum[].

It works.  Now let me fire up my time machine and undo one of my posts
from earlier today ...

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

Re: [PATCH] - prof_cpu_mask problems

[Paul Jackson]

pj wrote:
> There will be a single routine for each, in a new lib file.

Bill replied:
> The format_cpumask() bits did that in ...

Yup, as I knew, before and after that point in time.  Just not then.
The older I get, the more I appreciate how non-linear awareness is.

> I'm not convinced it does. 

You're right - the code seems ok on this point.

> I don't see any problems with changing the format at this stage ...

Ok - I will continue along this path ... at least until when and
if someone raises a good objection.

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