Re: [PATCH] [genirq] Expose default irq affinity mask (take 2)

From: Max Krasnyanskiy
Date: Thu May 29 2008 - 12:31:15 EST


Paul Jackson wrote:
Review comments ... just nits ... patch looks good overall.

1. Trailing whitespace on Doc lines:
+irq subdir is one subdir for each IRQ, and two files; default_smp_affinity and +smp_affinity is a bitmask, in which you can specify which CPUs can handle the +default_smp_affinity mask applies to all non-active IRQs. In other words IRQs

(Granted, there are already about 300 hundred trailing whitespaces in that file,
so this is like trying not to spit into the Pacific ocean ;).

2. I think that the following line should be removed from kernel/irq/proc.c:

+extern cpumask_t irq_default_affinity;

(You already pick up that extern from <linux/irq.h>, as it should be.)

3. The trailing period was missing after the line:

irq subdir is one subdir for each IRQ, and one file; prof_cpu_mask

Might as well add it, after "prof_cpu_mask", while you're there anyway.

4. In the doc:

+ > echo 1 > /proc/irq/5/smp_affinity
+
+This means that only the first CPU will handle the IRQ, but you can also echo
+5 which means that only the first and fourth CPU can handle the IRQ.

it might be a good idea to not use "5" for both the example CPU
and for the example mask. It increases the chance of a reader
getting confused as to which 5 is which. Perhaps try a different
example CPU, such as:

echo 1 > /proc/irq/2/smp_affinity

5. The original Doc text, before your patch, stated that the
"The contents of the prof_cpu_mask file ... by default."

After your patch, this statement that the default contents
of the prof_cpu_mask file was ffffffff got lost.
6. The grammar of the following got mangled a little:

+default_smp_affinity mask applies to all non-active IRQs. In other words IRQs
+that have not been allocated/activated yet (for which /proc/irq/[0-9]* directory
+does not exist).

Perhaps the following is better:

+The default_smp_affinity mask applies to all non-active IRQs, which are the
+IRQs which have not yet been allocated/activated, and hence which lack a
+/proc/irq/[0-9]* directory.
7. The lines of code (with their definition of cpumask_t tmp):
+ cpus_and(tmp, new_value, cpu_online_map);
+ if (cpus_empty(tmp))
+ return -EINVAL;

can be better written, I'm pretty sure, as:
+ if (!cpus_intersects(new_value, cpu_online_map))
+ return -EINVAL;

Minimizing cpumask_t's as local stack variables is a good thing,
especially on some of the high (thousands) count CPU systems that
SGI is developing.

Nice (thorough) review. Thanx. I'll update the patch based on your comments.
#7 was definitely "monkey see, monkey do". I cut and pasted it from existing mask update without much thought. Will update both places.

Thanx
Max


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/