From owner-svn-src-all@FreeBSD.ORG Tue Aug 10 11:03:53 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 5178C1065672; Tue, 10 Aug 2010 11:03:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail05.syd.optusnet.com.au (mail05.syd.optusnet.com.au [211.29.132.186]) by mx1.freebsd.org (Postfix) with ESMTP id B4DBB8FC17; Tue, 10 Aug 2010 11:03:52 +0000 (UTC) Received: from c122-106-147-41.carlnfd1.nsw.optusnet.com.au (c122-106-147-41.carlnfd1.nsw.optusnet.com.au [122.106.147.41]) by mail05.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o7AB3gqP029029 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 10 Aug 2010 21:03:44 +1000 Date: Tue, 10 Aug 2010 21:03:42 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: "M. Warner Losh" In-Reply-To: <20100809.111922.276219111637867136.imp@bsdimp.com> Message-ID: <20100810202538.G10110@delplex.bde.org> References: <20100809.111922.276219111637867136.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="0-1132955669-1281438222=:10110" X-Mailman-Approved-At: Tue, 10 Aug 2010 11:16:26 +0000 Cc: src-committers@FreeBSD.org, jhb@FreeBSD.org, c.jayachandran@gmail.com, 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: Tue, 10 Aug 2010 11:03:53 -0000 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-1132955669-1281438222=:10110 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE On Mon, 9 Aug 2010, M. Warner Losh wrote: > In message: > "Jayachandran C." writes: > : > 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 al= l > : > our architectures. > : > That is why we are going to use cpuset_t for dealing with CPUs number= s > : > (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. To avoid the pitfall, the 32 in the latest should be spelled something like (sizeof(cpu_mask_t) * CHAR_BIT). Perhaps some literal constants also need to be cast to cpu_mask_t. > 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) This spelling of 32 is better than the hard 32. >>>>> + =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 > > =09if (mp_ncpus > sizeof(top->cg_mask) * NBBY) > =09=09mp_ncpus =3D sizeof(top->cg_mask) * NBBY; /* Or panic */ When fixing the spelling of sizeof(char), please catch up with C90 and spell it CHAR_BIT. > =09if (mp_ncpus > sizeof(top->cg_mask) * NBBY) > =09=09top->cg_mask =3D ~0;=09 /* which avoids the signed error */ > =09else > =09=09top->cg_mask =3D (1 << mp_ncpus) - 1; This is missing a cast of 1. With 16-bit ints, this would overflow even with a 32-bit cpumask_t. > I'm not sure why the expression would fail (1 << 32 =3D=3D 0 -1 =3D=3D al= l > bits set), but I've not looked at the old thread to see why the > compiler is generating bogus code. (1 << 32) gives undefined behaviour unless ints have at least 33 value bits (and 1 sign bit). IIRC, even within the x86 family, some actual behaviours are to give a result of either 0 or 1, depending on whether the shift count is masked with 0x1F before shifting. Even (1 << 31) with normal 32-bit ints gives undefined behaviour. C90 says that it gives the result of shifting, but that is unpredictable and nothing is required for it. C99 fixes this and makes it explicitly undefined (since normal 32-bit ints have 31 value bits and (1 << 31) requires at least 32 value bits to represent in the type of the left operand 1, i.e., int). The actual behaviour for broken expressions like =09=09top->cg_mask =3D (1 << mp_ncpus) - 1; is: - first (1 << 31) overflows benignly to the negative value INT_MIN (with normal 2's complement 32-bit ints) - next, INT_MIN - 1 overflows benignly to the positive value INT_MAX - the signed value INT_MAX is assigned to the unsigned variable cg_mask. Since INT_MAX is positive and has a type smaller than cg_mask, there are no further overflows or other significant conversions - long mails about this are sent by language lawyers - the overflows accidentally resulted in the correct value, so there are no further problems. Bruce --0-1132955669-1281438222=:10110--