lkml.org 
[lkml]   [2008]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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




\
 
 \ /
  Last update: 2008-05-29 18:33    [W:0.129 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site