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>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <AANLkTin9tn+OFOkQSYSewtgVQrDNoa2gE2SqPxA73cih@mail.gmail.c=
om>
"Jayachandran C." <c.jayachandran@gmail.com> writes:
: >>>> =A0 =A0 =A0 =A0top->cg_level =3D 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 bet=
ween
: >>> 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 th=
is
: >> 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 cpumas=
k_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 numb=
ers
: > (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:
>>>> - =A0 =A0 =A0 top->cg_mask =3D (1 << mp_ncpus) - 1;
>>>> + =A0 =A0 =A0 if (mp_ncpus =3D=3D sizeof(top->cg_mask) * 8)
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 top->cg_mask =3D -1;
>>>> + =A0 =A0 =A0 else
>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 top->cg_mask =3D (1 << mp_ncpus) - 1=
;
but rather
if (mp_ncpus > sizeof(top->cg_mask) * NBBY)
mp_ncpus =3D sizeof(top->cg_mask) * NBBY; /* Or panic */
if (mp_ncpus > sizeof(top->cg_mask) * NBBY)
top->cg_mask =3D ~0; /* which avoids the signed error */
else
top->cg_mask =3D (1 << mp_ncpus) - 1;
I'm not sure why the expression would fail (1 << 32 =3D=3D 0 -1 =3D=3D =
all
bits set), but I've not looked at the old thread to see why the
compiler is generating bogus code.
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100809.111922.276219111637867136.imp>