From owner-svn-src-all@FreeBSD.ORG Sat Jul 30 23:21:53 2011
Return-Path: <owner-svn-src-all@FreeBSD.ORG>
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 <brde@optusnet.com.au>
X-X-Sender: bde@besplex.bde.org
To: Bruce Evans <brde@optusnet.com.au>
In-Reply-To: <20110731081349.K1061@besplex.bde.org>
Message-ID: <20110731091237.R1216@besplex.bde.org>
References: <201107301333.p6UDX5ca001997@svn.freebsd.org>
<D7B5C4D2-350D-43FA-9E40-DCE166281F7D@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" <bz@FreeBSD.org>,
John Baldwin <jhb@FreeBSD.org>, 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" \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all>
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=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