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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20080528233712.5a95b63e.pj@sgi.com>
Date:	Wed, 28 May 2008 23:37:12 -0500
From:	Paul Jackson <pj@....com>
To:	Max Krasnyansky <maxk@...lcomm.com>
Cc:	mingo@...e.hu, a.p.zijlstra@...llo.nl,
	linux-kernel@...r.kernel.org, tglx@...utronix.de,
	rdunlap@...otime.net, maxk@...lcomm.com
Subject: Re: [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 <pj@....com> 1.940.382.4214
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/