From owner-svn-src-all@FreeBSD.ORG Sat Jul 30 23:21:53 2011 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 0C156106566B; Sat, 30 Jul 2011 23:21:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 937E98FC0C; Sat, 30 Jul 2011 23:21:52 +0000 (UTC) Received: from c122-106-165-191.carlnfd1.nsw.optusnet.com.au (c122-106-165-191.carlnfd1.nsw.optusnet.com.au [122.106.165.191]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p6UNLjsJ001468 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 31 Jul 2011 09:21:48 +1000 Date: Sun, 31 Jul 2011 09:21:45 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Bruce Evans In-Reply-To: <20110731081349.K1061@besplex.bde.org> Message-ID: <20110731091237.R1216@besplex.bde.org> References: <201107301333.p6UDX5ca001997@svn.freebsd.org> <20110731081349.K1061@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, "Bjoern A. Zeeb" , John Baldwin , src-committers@FreeBSD.org Subject: Re: svn commit: r224516 - in head/sys: amd64/amd64 i386/i386 pc98/pc98 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: Sat, 30 Jul 2011 23:21:53 -0000 On Sun, 31 Jul 2011, Bruce Evans wrote: > ... In the > whole kernel, 155 lines match TUNABLE.*FETCH and 14 of these match 'if ('. > Some of the 14 are not wrong. About 1/3 of them are for dubious use in > memory sizing. E.g., in amd64/machdep.c: > > % realmem = Maxmem; > > The unprobed Maxmem remains in realmem for printing a confusing value later > in sysctls. Oops. This is actually the final (probed) value of Maxmem. I got confused by the file order being different from the dynamic order. > > % % /* > % * Display physical memory if SMBIOS reports reasonable amount. > % */ > % memsize = 0; > % sysenv = getenv("smbios.memory.enabled"); > % if (sysenv != NULL) { > % memsize = (uintmax_t)strtoul(sysenv, (char **)NULL, 10) << > 10; > % freeenv(sysenv); > % } > > First override for `memsize'. > > % if (memsize < ptoa((uintmax_t)cnt.v_free_count)) > % memsize = ptoa((uintmax_t)Maxmem); > > `memsize' is normally (?) `Maxmem' in bytes, but not if SMBIOS set it. > > % printf("real memory = %ju (%ju MB)\n", memsize, memsize >> 20); > > Code doesn't match comment. The display is unconditional, but the comment > set that it is only done if SMBIOS reports a reasonable amount. > > `memsize' is not used after this point, so the only effect of the SMBIOS > check is to possibly print a value that is different from all of the > following: > - the initial Maxmem in bytes > - the final realmem in bytes (this equals the initial Maxmem in bytes) > - the final Maxmem in bytes. > The comment is also wrong in saying that SMBIOS is used. Only an > environment string is used. Users can easily confuse themselves by putting > garbage in this string. > > The above logic is also confused by the memory size being in cnt.v_free_cnt. > If SMBIOS does't change memsize from 0, then we use Maxmem even if it is > larger than cnt.v_free_cnt. So realmem is the correct final value unless the SMBIOS non-call gives a different value; in the latter case, we print this different value above but the realmem sysctl can't recover it. > % /* > % * Maxmem isn't the "maximum memory", it's one larger than the > % * highest page of the physical address space. It should be > % * called something like "Maxphyspage". We may adjust this > % * based on ``hw.physmem'' and the results of the memory test. > % */ > % Maxmem = atop(physmap[physmap_idx + 1]); > > Here we change Maxmem to another unprobed value. I think this value is > more closely related to cnt.v_free_cnt. > > % % #ifdef MAXMEM > % Maxmem = MAXMEM / 4; > % #endif > > Then we optionally change MaxMem to a hard-configured value. > > % % if (TUNABLE_ULONG_FETCH("hw.physmem", &physmem_tunable)) > % Maxmem = atop(physmem_tunable); > > Then we use a temporary variable, since the primary variable has > units of pages while the tunable has units of bytes. But this can > be reorganized to: > > /* > * memsize was Maxmem in bytes, but was not maintained when Maxmem > * was changed about. Maintain it now. > */ > memsize = ptoa(Maxmem). > * > * XXX we also have a physmem variable, but this can't be used to > * hole the result of the tunable any more than Maxmem can, since > * it is in pages. memsize is not quite right either, since it > * is a uintmax_t while the following assumes that it is u_long. > * The u_long is too small for PAE on i386, but works for amd64. > * The uintmax_t is needed on i386, but is not needed for amd64. > */ > TUNABLE_ULONG_FETCH("hw.physmem", &memsize); /* XXX sic */ > Maxmem = atop(memsize); > > % /* > % * Don't allow MAXMEM or hw.physmem to extend the amount of memory > % * in the system. > % */ > % if (Maxmem > atop(physmap[physmap_idx + 1])) > % Maxmem = atop(physmap[physmap_idx + 1]); Now we're in the getmemsize() function, which doesn't have a memsize variable. It should have one instead of the physmem_tunable variable, if only because the physmem tunable is actually for memsize in bytes and not for physmem in pages. Bruce