From owner-svn-src-all@FreeBSD.ORG Thu Nov 8 13:50:52 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id F3336E6C; Thu, 8 Nov 2012 13:50:51 +0000 (UTC) (envelope-from marius@alchemy.franken.de) Received: from alchemy.franken.de (alchemy.franken.de [194.94.249.214]) by mx1.freebsd.org (Postfix) with ESMTP id 54ABA8FC12; Thu, 8 Nov 2012 13:50:50 +0000 (UTC) Received: from alchemy.franken.de (localhost [127.0.0.1]) by alchemy.franken.de (8.14.4/8.14.4/ALCHEMY.FRANKEN.DE) with ESMTP id qA8DomuF076623; Thu, 8 Nov 2012 14:50:48 +0100 (CET) (envelope-from marius@alchemy.franken.de) Received: (from marius@localhost) by alchemy.franken.de (8.14.4/8.14.4/Submit) id qA8DomhL076622; Thu, 8 Nov 2012 14:50:48 +0100 (CET) (envelope-from marius) Date: Thu, 8 Nov 2012 14:50:48 +0100 From: Marius Strobl To: Bruce Evans Subject: Re: svn commit: r242747 - head/sys/kern Message-ID: <20121108135048.GQ80490@alchemy.franken.de> References: <201211080810.qA88AW8Y027373@svn.freebsd.org> <20121108205636.O3941@besplex.bde.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20121108205636.O3941@besplex.bde.org> User-Agent: Mutt/1.4.2.3i Cc: svn-src-head@freebsd.org, Marius Strobl , src-committers@freebsd.org, svn-src-all@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 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: Thu, 08 Nov 2012 13:50:52 -0000 On Thu, Nov 08, 2012 at 09:41:29PM +1100, Bruce Evans wrote: > On Thu, 8 Nov 2012, Marius Strobl wrote: > > >Log: > > Make r242655 build on sparc64. While at it, make > > vm_{max,min}_kernel_address > > vm_offset_t as they should be. > > Er, they shouldn't be vm_offset_t. > > >Modified: head/sys/kern/kern_malloc.c > >============================================================================== > >--- head/sys/kern/kern_malloc.c Thu Nov 8 04:02:36 2012 (r242746) > >+++ head/sys/kern/kern_malloc.c Thu Nov 8 08:10:32 2012 (r242747) > >@@ -186,11 +186,13 @@ struct { > > */ > >static uma_zone_t mt_zone; > > > >-static u_long vm_min_kernel_address = VM_MIN_KERNEL_ADDRESS; > >+static vm_offset_t vm_min_kernel_address = VM_MIN_KERNEL_ADDRESS; > >SYSCTL_ULONG(_vm, OID_AUTO, min_kernel_address, CTLFLAG_RD, > > &vm_min_kernel_address, 0, "Min kernel address"); > > SYSCTL_ULONG takes a u_long, not a vm_offset_t. A cast in > SYSCTL_ULONG() prevents possible detection of the type mismatch from > this. > > This is most broken on i386's with correctly-sized longs (or almost > any arch with correctly-sized longs). There, vm_offset_t is 1 > register wide and longs are 2 registers wide. Eh, FreeBSD/i386 is ILP32, so longs are 32-bit there, as is its __vm_offset_t. > > The bugs could be moved using SYSCTL_QUAD(). Correctly-sized quads > would be 4 registers wide, except quad has come to mean just a bad > way of spelling long long or int64_t. > > >-static u_long vm_max_kernel_address = VM_MAX_KERNEL_ADDRESS; > >+#ifndef __sparc64__ > >+static vm_offset_t vm_max_kernel_address = VM_MAX_KERNEL_ADDRESS; > >+#endif > >SYSCTL_ULONG(_vm, OID_AUTO, max_kernel_address, CTLFLAG_RD, > > &vm_max_kernel_address, 0, "Max kernel address"); > > Since the value is a compile-time constant, u_long has a chance of > holding its value even if u_long != vm_offset_t, and the old code is > closer to being correct than first appeared, and the correct fix is > relatively easy -- just use a uintmax_t vatiable and SYSCTL_UINTMAX() > (*). Unfortunately SYSCT_UINTMAX() doesn't exist, and the bogus > SYSCTL_UQUAD() does exist. Right, SYSCTL_UINTMAX() unfortunately doesn't exist. SYSCTL_UQUAD() seemed inappropriate as it is 64-bit and we want 32-bit for ILP32 for an exact match. Using uintmax_t with SYSCTL_UQUAD() also just happens to be of the same width. >From the available combinations, using vm_offset_t with SYSCTL_ULONG() just seemed to suck the least. > > (*) Except a temporary variable is just a style bug in the above and > for this. SYSCTL_UNLONG(), like all integer SYSCTL()'s, has the feature > of supporting either a variable or a constant arg. The above should > use a constant arg and not need a variable. IIRC, the syntax for this is: > > SYSCTL_ULONG(_vm, OID_AUTO, max_kernel_address, CTLFLAG_RD, > NULL, VM_MAX_KERNEL_ADDRESS, "Max kernel address"); Actually, for sparc64 VM_MAX_KERNEL_ADDRESS isn't constant (and can't be). > > subr_param.c is careful to use only basic types for all of its variables > so that standard sysctls apply. > > Tunables have even more bugs in this area: at least at the input level > kern_environment.c: > - bogus quads are supported, but bogus uquads aren't > - everything goes through getenv_quad(), which uses strtoq(), which is > for signed values, so unsigned values are mishandled in various ways > - worst sign bugs are on 64-bit arches. getenv_ulong() uses getenv_quad(), > with blind casts and no overflow checking, so 64-bit values are silently > truncated to QUAD_MAX (63 bits). Before that, getenv_quad() has overflow > checking in its strtoq() call, but none in its multiplication, and the > API is broken (strtoq(3) indicatates errors in errno, but there is no > errno in the kernel). > > The bugs in tunables mean that sysctls that report read-only tunables > can safely use SYSCTL_QUAD(). > > This is hard to fix cleanly without combinatorial explosion in the number > of types, giving uglyness in the implementation. It is probably best to > have single SYSCTL_INT() that operates on all integral types (signed > and unsigned) according to its the arg type. E.g.: > > static vm_offset_t vm_min_kernel_address = VM_MIN_KERNEL_ADDRESS; > SYSCTL_INT(_vm, OID_AUTO, min_kernel_address, CTLFLAG_RD, > &vm_min_kernel_address, 0, "Min kernel address"); > > This should expand to an uncast &vm_min_kernel_address, with auxilary > data for the size and sign of that. sysctl_handle_int() should operate > atomically on all integer sizes according to the size and sign info. > Some SYSCTL_FOO()'s already generate size info or take a size arg, but > sysctl_handle_int() only supports plain int and is abused to support > u_int and is sometimes misused on sysctl data that has a size. > > After fixing the style bug: > > SYSCTL_INT(_vm, OID_AUTO, min_kernel_address, CTLFLAG_RD, > NULL, VM_MIN_KERNEL_ADDRESS, "Min kernel address"); > > The constant can have any integral type, and a the sysctl must generate > size and sign info for it too. > > The API can probably be simplified to reduce the 2 parameters to 1. > Hopefully __builtin_constant_p() can be used to decide which case applies. > Marius