[lkml]   [2008]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] [genirq] Expose default irq affinity mask (take 2)
    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 work. Thanks.

    I won't rest till it's the best ...
    Programmer, Linux Scalability
    Paul Jackson <> 1.940.382.4214

     \ /
      Last update: 2008-05-29 06:55    [W:0.022 / U:219.532 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site