svn commit: r208165 - in head/sys: kern mips/conf mips/include mips/mips mips/rmi mips/rmi/dev/xlr

[M. Warner Losh]

In message: <AANLkTin9tn+OFOkQSYSewtgVQrDNoa2gE2SqPxA73cih at mail.gmail.com>
            "Jayachandran C." <c.jayachandran at gmail.com> writes:
: >>>>        top->cg_level = CG_SHARE_NONE;
: >>>>
: >>>
: >>> ... and this is why I particulary hate big commits with complete lack
: >>> of technical details.
: >>>
: >>> This particulary chunk was supposed to fix a nasty and completely MI
: >>> bug that some users have already met (kern/148698). The complete lack
: >>> of details didn't help in identify the issue neither that it was a
: >>> valuable fix.
: >>>
: >>> The fix is, however, improper (there is no clear relationship between
: >>> the multiplication and why that happens) thus I would rather use what
: >>> Joe has reported in the PR.
: >>
: >>
: >> I was not aware of the PR when I sent this changes to rrs@, and this
: >> went as a part of MIPS SMP support for XLR which has 32 CPUs
: >>
: >> But I'm not sure that the current change is correct, cg_mask is of
: >> type cpumask_t and I don't think it is guaranteed to be 32 bit (as it
: >> is a machine dependent type).
: >
: > Actually the 32 bits limit is well aware and acknowledged in cpumask_t.
: > While it is true that it should be an 'opaque' type, in reality it is
: > not. The maximum limit of 32 CPUs is a reality due to cpumask_t on all
: > our architectures.
: > That is why we are going to use cpuset_t for dealing with CPUs numbers
: > (which is really opaque, at same extent, and is not bound to any
: > size).
: 
: In my opinion, your changes added another pitfall for the person who
: tries to make the cpumask_t 64 bit to support more cpus, which really
: could have been avoided.
: 
: But if cpumask_t is going to be changed to cpuset_t soon, I guess we
: are fine  :)
: 
: JC.

Yes, the correct fix, short of cpuset_t, is not:

>>>> -       top->cg_mask = (1 << mp_ncpus) - 1;
>>>> +       if (mp_ncpus == sizeof(top->cg_mask) * 8)
>>>> +               top->cg_mask = -1;
>>>> +       else
>>>> +               top->cg_mask = (1 << mp_ncpus) - 1;

but rather

	if (mp_ncpus > sizeof(top->cg_mask) * NBBY)
		mp_ncpus = sizeof(top->cg_mask) * NBBY; /* Or panic */
	if (mp_ncpus > sizeof(top->cg_mask) * NBBY)
		top->cg_mask = ~0;	    /* which avoids the signed error */
	else
		top->cg_mask = (1 << mp_ncpus) - 1;


I'm not sure why the expression would fail (1 << 32 == 0 -1 == all
bits set), but I've not looked at the old thread to see why the
compiler is generating bogus code.

Warner