Date: Mon, 09 Aug 2010 11:19:22 -0600 (MDT) From: "M. Warner Losh" <imp@bsdimp.com> To: c.jayachandran@gmail.com Cc: src-committers@freebsd.org, jhb@freebsd.org, jchandra@freebsd.org, svn-src-all@freebsd.org, jlanders@vmware.com, attilio@freebsd.org, rrs@freebsd.org, sbruno@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r208165 - in head/sys: kern mips/conf mips/include mips/mips mips/rmi mips/rmi/dev/xlr Message-ID: <20100809.111922.276219111637867136.imp@bsdimp.com> In-Reply-To: <AANLkTin9tn%2BOFOkQSYSewtgVQrDNoa2gE2SqPxA73cih@mail.gmail.com> References: <AANLkTi=jkUiXStPV3Gq206GgmctM9%2B1ofbNWuh5CSeWw@mail.gmail.com> <AANLkTim_pTQgquRRuz4G=jC5G0Amc%2ByF6VeDcg-cpTCX@mail.gmail.com> <AANLkTin9tn%2BOFOkQSYSewtgVQrDNoa2gE2SqPxA73cih@mail.gmail.com>
index | next in thread | previous in thread | raw e-mail
In message: <AANLkTin9tn+OFOkQSYSewtgVQrDNoa2gE2SqPxA73cih@mail.gmail.com>
"Jayachandran C." <c.jayachandran@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
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100809.111922.276219111637867136.imp>