LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mike Travis <travis@sgi.com>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: Ingo Molnar <mingo@elte.hu>, Andi Kleen <andi@firstfloor.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	davej@codemonkey.org.uk, David Miller <davem@davemloft.net>,
	Eric Dumazet <dada1@cosmosbay.com>,
	Jack Steiner <steiner@sgi.com>,
	Jeremy Fitzhardinge <jeremy@goop.org>, Jes Sorensen <jes@sgi.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	peterz@infradead.org, Thomas Gleixner <tglx@linutronix.de>,
	Yinghai Lu <yhlu.kernel@gmail.com>,
	IA64 <linux-ia64@vger.kernel.org>,
	PowerPC <linuxppc-dev@ozlabs.org>,
	S390 <linux-s390@vger.kernel.org>,
	SPARC <sparclinux@vger.kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 18/35] cpumask: add nr_cpumask_bits
Date: Tue, 21 Oct 2008 06:53:04 -0700	[thread overview]
Message-ID: <48FDDEC0.9040903@sgi.com> (raw)
In-Reply-To: <200810212326.10278.rusty@rustcorp.com.au>

Rusty Russell wrote:
> On Tuesday 21 October 2008 04:03:37 Mike Travis wrote:
>> When nr_cpu_ids is set to CONFIG_NR_CPUS then references to nr_cpu_ids
>> will return the maximum index of the configured NR_CPUS (+1) instead
>> of the maximum index of the possible number of cpus (+1).  This results
>> in extra unused memory being allocated by functions that are setting up
>> arrays of structs to keep track of per cpu items.
> 
> 1) I like the name in this context: it's a beacon of sanity after NR_CPUS and
>    nr_cpu_ids.  But it's not so clearly a win when general code uses it:
> 
> 	if (cpumask_first(mymask) == nr_cpumask_bits) ...
> 
>    vs:
>    
> 	if (cpumask_first(mymask) == nr_cpu_ids) ...

I think the correct use for iterators would be:

	if (cpumask_first(mymask) >= nr_cpu_ids)

... since nr_cpu_ids is guaranteed to be <= nr_cpumask_bits.  And nr_cpumask_bits
is not really meant to be used anywhere except in the bit operations supporting
the cpumask_* operators.

> 2) This breaks anyone who tests that the iterators etc. return == nr_cpu_ids.

I think that's broken anyways... ;-)

>    One of the other patches tried to change them from NR_CPUS to nr_cpu_ids,
>    that should now be revisited & reaudited.

The change from NR_CPUS to nr_cpu_ids is ok, but it should also be changed from:

	(x == NR_CPUS)
to:
	(x <= nr_cpu_ids)
 
> 3) Noone should be naively allocating "* nr_cpu_ids" arrays, they should be
>    using per-cpu pointers.  Not doing so wastes memory on non-contiguous
>    processor systems.

The problem often arises where an array is allocated that will use the
cpu as an index into the array.  They can be changed eventually to use a
percpu pointer, but in the interim keeping nr_cpu_ids intact maintains
compatibility without allocating unused memory.

> 4) It should be a constant not be dependent on CONFIG_CPUMASK_OFFSTACK, but
>    rather as it was on NR_CPUS > BITS_PER_LONG.  I think that's the sweet
>    spot, and should also make your 2MB "gain" vanish.

I'll run the test again but most likely the result will be an extra 1Mb of
unused memory instead. ;-)  One other note, that test compile used the default
config [and NR_CPUS=128] which turns off a lot of functions.  A typical distro
config will have many more options turned on.

And the beauty of using a separate flag to enable variable length cpumasks,
is there may be cases where an arch or specific system config wants a multiple
word cpumask on the stack for performance reasons (like cache or node locality,
avoidance of the kmalloc's, etc.)

> That's why I suggested a max_possible_cpu() function, and using that for those 
> who really want to do allocations, who should be audited anyway, see (3).  I 
> don't want it as prominent as nr_cpu_ids, which is usually the Right Thing, 
> and always safe.

We could change all refs from nr_cpu_ids to max_possible_cpu but wouldn't
we still be leaving a window open where it could be incorrectly used?
So far all the cpumask conversions allow for "mixed-use" cpumask (i.e.,
cpumask_t and struct cpumask *) by maintaining backwards compatility,
until everything is eventually sorted out.  Keeping nr_cpu_ids representing
the same value maintains this "compatibility bridge".

The most correct way would be to use the (not yet implemented) zero
based PERCPU allocator.  Slightly less efficient would be to allocate
node local memory for each struct, in a loop using a per cpu pointer
and "for_each_possible_cpu()".

> Cheers,
> Rusty.
> PS.  I have part of a patch for this...

But as I've said, it's not critical to the new functionality...

Thanks!
Mike

  reply	other threads:[~2008-10-21 13:52 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-10-20 17:03 [PATCH 00/35] cpumask: Replace cpumask_t with struct cpumask Mike Travis
2008-10-20 17:03 ` [PATCH 01/35] x86: clean up speedctep-centrino and reduce cpumask_t usage Mike Travis
2008-10-21  0:09   ` Stephen Rothwell
2008-10-21 12:28     ` Mike Travis
2008-10-20 17:03 ` [PATCH 02/35] cpumask: remove min from first_cpu/next_cpu Mike Travis
2008-10-20 17:03 ` [PATCH 03/35] cpumask: add for_each_cpu_mask_and function Mike Travis
2008-10-20 17:03 ` [PATCH 04/35] x86 smp: modify send_IPI_mask interface to accept cpumask_t pointers Mike Travis
2008-10-20 17:03 ` [PATCH 05/35] sched: Reduce stack size requirements in kernel/sched.c Mike Travis
2008-10-20 17:03 ` [PATCH 06/35] cpumask: introduce struct cpumask Mike Travis
2008-10-20 17:03 ` [PATCH 07/35] cpumask: change cpumask_scnprintf, cpumask_parse_user, cpulist_parse, and cpulist_scnprintf to take pointers Mike Travis
2008-10-20 17:03 ` [PATCH 08/35] cpumask: cpumask_size() Mike Travis
2008-10-20 17:03 ` [PATCH 09/35] cpumask: add cpumask_copy() Mike Travis
2008-10-20 17:03 ` [PATCH 10/35] cpumask: introduce cpumask_var_t for local cpumask vars Mike Travis
2008-10-20 17:03 ` [PATCH 11/35] x86: enable MAXSMP Mike Travis
2008-10-20 17:03 ` [PATCH 12/35] cpumask: make CONFIG_NR_CPUS always valid Mike Travis
2008-10-20 17:03 ` [PATCH 13/35] cpumask: use setup_nr_cpu_ids() instead of direct assignment Mike Travis
2008-10-20 17:03 ` [PATCH 14/35] cpumask: make nr_cpu_ids valid in all configurations Mike Travis
2008-10-20 17:03 ` [PATCH 15/35] cpumask: prepare for iterators to only go to nr_cpu_ids Mike Travis
2008-10-20 17:03 ` [PATCH 16/35] percpu: fix percpu accessors to potentially !cpu_possible() cpus Mike Travis
2008-10-20 17:03 ` [PATCH 17/35] cpumask: make nr_cpu_ids the actual limit on bitmap size Mike Travis
2008-10-20 17:03 ` [PATCH 18/35] cpumask: add nr_cpumask_bits Mike Travis
2008-10-21 12:26   ` Rusty Russell
2008-10-21 13:53     ` Mike Travis [this message]
2008-10-20 17:03 ` [PATCH 19/35] cpumask: use cpumask_bits() everywhere Mike Travis
2008-10-20 17:03 ` [PATCH 20/35] cpumask: cpumask_of(): cpumask_of_cpu() which returns a pointer Mike Travis
2008-10-20 17:03 ` [PATCH 21/35] cpumask: for_each_cpu(): for_each_cpu_mask which takes " Mike Travis
2008-10-20 17:03 ` [PATCH 22/35] cpumask: cpumask_first/cpumask_next Mike Travis
2008-10-20 17:03 ` [PATCH 23/35] cpumask: deprecate any_online_cpu() in favour of cpumask_any/cpumask_any_and Mike Travis
2008-10-20 17:03 ` [PATCH 24/35] cpumask: cpumask_any_but() Mike Travis
2008-10-20 17:03 ` [PATCH 25/35] cpumask: Deprecate CPUMASK_ALLOC etc in favor of cpumask_var_t Mike Travis
2008-10-20 17:03 ` [PATCH 26/35] cpumask: get rid of boutique sched.c allocations, use cpumask_var_t Mike Travis
2008-10-20 17:03 ` [PATCH 27/35] cpumask: to_cpumask() Mike Travis
2008-10-20 17:03 ` [PATCH 28/35] cpumask: accessors to manipulate possible/present/online/active maps Mike Travis
2008-10-20 17:03 ` [PATCH 29/35] cpumask: Use accessors code Mike Travis
2008-10-20 17:03 ` [PATCH 30/35] cpumask: CONFIG_BITS_ALL, CONFIG_BITS_NONE and CONFIG_BITS_CPU0 Mike Travis
2008-10-20 17:03 ` [PATCH 31/35] cpumask: switch over to cpu_online/possible/active/present_mask Mike Travis
2008-10-20 17:03 ` [PATCH 32/35] cpumask: cpu_all_mask and cpu_none_mask Mike Travis
2008-10-20 17:03 ` [PATCH 33/35] cpumask: reorder header to minimize separate #ifdefs Mike Travis
2008-10-20 17:03 ` [PATCH 34/35] cpumask: debug options for cpumasks Mike Travis
2008-10-20 17:03 ` [PATCH 35/35] cpumask: smp_call_function_many() Mike Travis

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=48FDDEC0.9040903@sgi.com \
    --to=travis@sgi.com \
    --cc=akpm@linux-foundation.org \
    --cc=andi@firstfloor.org \
    --cc=dada1@cosmosbay.com \
    --cc=davej@codemonkey.org.uk \
    --cc=davem@davemloft.net \
    --cc=hpa@zytor.com \
    --cc=jeremy@goop.org \
    --cc=jes@sgi.com \
    --cc=linux-ia64@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=mingo@elte.hu \
    --cc=peterz@infradead.org \
    --cc=rusty@rustcorp.com.au \
    --cc=sparclinux@vger.kernel.org \
    --cc=steiner@sgi.com \
    --cc=tglx@linutronix.de \
    --cc=yhlu.kernel@gmail.com \
    --subject='Re: [PATCH 18/35] cpumask: add nr_cpumask_bits' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).