Messages in this thread | | | Date | Thu, 29 May 2008 09:30:56 -0700 | From | Max Krasnyanskiy <> | Subject | Re: [PATCH] [genirq] Expose default irq affinity mask (take 2) |
| |
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
| |