Re: [patch and review please] 64 CPU Support

On 2010-Jul-25 15:13:22 -0700, Sean Bruno <seanbru_at_yahoo-inc.com> wrote:
>On Sun, 2010-07-25 at 13:57 -0700, Kostik Belousov wrote:
>> Very low-priority comment (I looked at the patch at the time it was
>> posted to stable_at_). Nice thing about the patch is that it presumably
>> identifies all the places that depend on the wideness of the cpu
>> mask. Would it make sense to abstract the cpumask operations with
>> some macros to not repeat the search for the places when 64 will
>> be too narrow again ?
>
>What do you mean?  Can you give me a quick example?  

I can easily buy a machine with 128 CPU threads today (maybe 256).
At some point, FreeBSD is going to need to be able to support boxes
that contain too many CPUs to hold the CPU mask in a long.

Your patch pulls (1 << i) into a macro cputomask(i) but still assumes
that a CPU mask is representable as an integral type.  Since you've
gone to the effort of identifying all the places where this cpumask
operations are referenced, would it not make more sense to replace
them all with more generic functions to simplify future support for
more than 64 CPUs.

As a concrete example, instead of:
 	CPU_FOREACH(i) {
-		if (((1 << i) & map) != 0)
+		if ((cputomask(i) & map) != 0)
 			ncpus++;
 	}
use something like:
 	CPU_FOREACH(i) {
		if (cpu_testmask(&map, i))
 			ncpus++;
 	}

And instead of:
 	pcpu->pc_cpuid = cpuid;
-	pcpu->pc_cpumask = 1 << cpuid;
+	pcpu->pc_cpumask = cputomask(cpuid);
 	cpuid_to_pcpu[cpuid] = pcpu;
something like:
 	pcpu->pc_cpuid = cpuid;
	cpu_setmask(&pcpu->pc_cpumask, cpuid);
 	cpuid_to_pcpu[cpuid] = pcpu;

Obviously, expanding the atomic operations beyond long is non-trivial
but I suspect that the requirement is only that each individual bit is
atomic between the "master" CPU and the relevant other CPU - which
means that the atomic operations can be carried out independently on a
suitable subset (int or long) of the complete mask.

-- 
Peter Jeremy