- prof_cpu_mask problems

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

@@ -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 ([email protected])          651-683-5302
Principal Engineer                      SGI - Silicon Graphics, Inc.

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

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

Matthew wrote (on the linux-ia64 list):

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 <[email protected]> 1.650.933.1373

Matthew wrote (on the linux-ia64 list):

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

ah - ok - thanks.

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 <[email protected]> 1.650.933.1373

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 <[email protected]> 1.650.933.1373

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.

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.

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

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 <[email protected]> 1.650.933.1373

Bill replied:

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.

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

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 <[email protected]> 1.650.933.1373