linux-kernel - Re: [PATCH 05/31] cpumask: Provide new cpumask API

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: <48E248CB.5080305@sgi.com>
Date:	Tue, 30 Sep 2008 08:42:03 -0700
From:	Mike Travis <travis@....com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	Rusty Russell <rusty@...tcorp.com.au>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Miller <davem@...emloft.net>,
	Yinghai Lu <yhlu.kernel@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jack Steiner <steiner@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/31] cpumask: Provide new cpumask API

Ingo Molnar wrote:
> * Mike Travis <travis@....com> wrote:
> 
>>     /* replaces cpumask_t dst = (cpumask_t)src */
>>     void cpus_copy(cpumask_t dst, const cpumask_t src);
> 
> minor namespace nit i noticed while looking at actual usage of 
> cpus_copy(): could you please rename it cpumask_set(dst, src)?
> 
> That streamlines it to have the same naming concept as atomic_set(), 
> node_set(), zero_fd_set(), etc.

Cpus_copy came from it's underlying function: bits_copy().  Cpumask_set
would deviate from the current naming convention of cpu_XXX for single
cpu ops and cpus_XXX for all cpus ops.  Do we want that?

> 
> the patch-set looks quite nice otherwise already, the changes are 
> straightforward and the end result already looks a lot more maintainable 
> and not fragile at all.

I was hoping for a stronger compiler error to indicate incorrect usage,
it currently just says "may be used before it's initialized" if you mistakenly
have cpumask_t as the local variable declaration.  I'm testing it now.

> 
> In what way will Rusty's changes differ? Since you incorporate some of 
> Rusty's changes already, could you please iterate towards a single 
> patchset which we could then start testing?

Our timezones are not very conducive to a lot of email exchanges (and he's moving.)
>From what I've seen I believe he's leaning towards using struct cpumask * and
less trickery than I have.

The other alternative that is very easy to implement with the new code is
using a simple unsigned long list for cpumask_t (as Linus first suggested):

	typedef unsigned long cpumask_var_t[1];	/* small NR_CPUS */
	typedef unsigned long *cpumask_var_t;	/* large NR_CPUS */

This simplifies things quite a bit and I can get rid of some trickery (and
it's a pointer already without having to invent a pointer to a struct.)  The
downside is arrays of cpumask_t's are less clean, but doable.  The best thing
about the changes in this patchset is the context of the cpumask is more well
known, and changes to the underlying type for cpumask are confined to only a
few files (cpumask.h, lib/cpumask.c, etc.)

Thanks,
Mike
--
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/