From owner-svn-src-all@FreeBSD.ORG Mon Aug 9 17:21:14 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 11B1C106567A; Mon, 9 Aug 2010 17:21:14 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id A0F408FC1E; Mon, 9 Aug 2010 17:21:13 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id o79HIpDZ058995; Mon, 9 Aug 2010 11:18:52 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Mon, 09 Aug 2010 11:19:22 -0600 (MDT) Message-Id: <20100809.111922.276219111637867136.imp@bsdimp.com> To: c.jayachandran@gmail.com From: "M. Warner Losh" In-Reply-To: References: X-Mailer: Mew version 6.3 on Emacs 22.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable X-Mailman-Approved-At: Mon, 09 Aug 2010 17:38:50 +0000 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 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Aug 2010 17:21:14 -0000 In message: "Jayachandran C." 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