In message: <aanlktin9tn+ofokqsysewtgvqrdnoa2ge2sqpxa73...@mail.gmail.com>
"Jayachandran C." <c.jayachand...@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
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
