Date: Sun, 31 Jul 2011 09:07:44 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, "Bjoern A. Zeeb" <bz@FreeBSD.org>, src-committers@FreeBSD.org Subject: Re: svn commit: r224516 - in head/sys: amd64/amd64 i386/i386 pc98/pc98 Message-ID: <20110731081349.K1061@besplex.bde.org> In-Reply-To: <D7B5C4D2-350D-43FA-9E40-DCE166281F7D@FreeBSD.org> References: <201107301333.p6UDX5ca001997@svn.freebsd.org> <D7B5C4D2-350D-43FA-9E40-DCE166281F7D@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 30 Jul 2011, John Baldwin wrote: > > On Jul 30, 2011, at 9:33 AM, Bjoern A. Zeeb wrote: > >> Author: bz >> Date: Sat Jul 30 13:33:05 2011 >> New Revision: 224516 >> URL: http://svn.freebsd.org/changeset/base/224516 >> >> Log: >> Introduce a tunable to disable the time consuming parts of bootup >> memtesting, which can easily save seconds to minutes of boot time. >> The tunable name is kept general to allow reusing the code in >> alternate frameworks. > > Why the 'tmpul' variable? The TUNABLE_*_FETCH() macros will not modify the parameter > if the environment variable is missing or unparseable. Lots of places depend on this all over > the kernel already, so you can depend on it here. And why do it and `memtest' have type unsigned long? This is a strange type for a boolean variable. The dependency is visible on there being almost no other instances of checking the return value of TUNABLE_*_FETCH(). E.g., in kern/*.c in a checkout a few months ago, there are 50 lines matching TUNABLE.*FETCH. Only 1 of these lines has an 'if (' in it. This 1 is in sysv_shm.c and seems to be correct since it is checking that a tunable which should not be used (because it has been renamed) is in fact not used. 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. % % /* % * 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. % /* % * 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]); After all these defaults and overrides for Maxmem, who knows its relationship to the "real memory" printed originally and printed later by sysctl? The memory size overrides and probes exist mainly to handle cases where the original value is wrong, so preserving the original value is a bug in most cases where it differs from the final value. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110731081349.K1061>