From owner-svn-src-all@FreeBSD.ORG Sat Jul 7 14:44:21 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 000A5106566B; Sat, 7 Jul 2012 14:44:20 +0000 (UTC) (envelope-from marcel@xcllnt.net) Received: from mail.xcllnt.net (mail.xcllnt.net [70.36.220.4]) by mx1.freebsd.org (Postfix) with ESMTP id C0F4B8FC0A; Sat, 7 Jul 2012 14:44:20 +0000 (UTC) Received: from dhcp-192-168-2-58.wifi.xcllnt.net (wifi.xcllnt.net [70.36.220.6] (may be forged)) (authenticated bits=0) by mail.xcllnt.net (8.14.5/8.14.5) with ESMTP id q67EiECC064629 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Sat, 7 Jul 2012 07:44:20 -0700 (PDT) (envelope-from marcel@xcllnt.net) Mime-Version: 1.0 (Apple Message framework v1278) Content-Type: text/plain; charset=us-ascii From: Marcel Moolenaar In-Reply-To: <20120707083535.GR2338@deviant.kiev.zoral.com.ua> Date: Sat, 7 Jul 2012 07:44:14 -0700 Content-Transfer-Encoding: quoted-printable Message-Id: <2B1DDEF4-A048-45BD-9A7A-8CB148282475@xcllnt.net> References: <201207061557.q66Fv45N069464@svn.freebsd.org> <20120706181213.GI2338@deviant.kiev.zoral.com.ua> <20120707083535.GR2338@deviant.kiev.zoral.com.ua> To: Konstantin Belousov X-Mailer: Apple Mail (2.1278) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Marcel Moolenaar , src-committers@freebsd.org Subject: Re: svn commit: r238172 - head/sys/dev/agp 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, 07 Jul 2012 14:44:21 -0000 On Jul 7, 2012, at 1:35 AM, Konstantin Belousov wrote: > On Fri, Jul 06, 2012 at 06:11:56PM -0700, Marcel Moolenaar wrote: >>=20 >> On Jul 6, 2012, at 11:12 AM, Konstantin Belousov wrote: >>=20 >>>> agp_i810.c: >>>> While arguably the use of Maxmem can be considered correct, replace = its use >>>> with realmem anyway. agp_i810.c is specific to amd64, i386 & pc98, = which >>>> have a dense physical memory layout. Avoiding Maxmem here is done = with an >>>> eye on copy-n-paste behaviour in general and to avoid confusion = caused by >>>> using realmem in agp.c and Maxmem in agp_i810.c. >>> The agp_i810.c use is to prevent attachment when largest physical = address >>> of populated memory exceeds GPU limits established by PTE format and >>> chipset errata. Editing Maxmem to be spelled as realmem seems to = change >>> nothing right now, but I do argue that this is wrong, and commit = message >>> makes future archeology quite confusing. >>=20 >> The commit log states it all, including how one can arguably call the = change >> wrong. What exactly is confusing? >=20 > The realmem is supposed to report available memory on the system, and > not the highest physical memory address. Correct. > Current calculation of maxmem > as realmem is already wrong, often by 1GB on typical desktop machine, > and I believe that it will become worse in the future. Sure. This is also why a variable like Maxmem is confusing. If you try to understand what it means or what it's for, you get mixed signals. On the one hand it's made identical to realmem and on the other hand there's comment that states that it's supposed to be the highest = address. > The platform does > has all capacity to report non-dense layouts to OS, and OS is capable = of > supporting them already. Yes, all platforms do. > I remember there were already reports of some > IBM machines which have sparce address space, making maxmem/(real = realmem) > be a factor of 2. That's nothing. On ia64 this can be a lot worse. > Confusing is the use of the amount of memory for decision that needs = highest > address. Commit log just restates the change made without any = motivation, > I think it is backward. The commit log states the motivation: developers tend to copy-n-paste = without truly understanding the subtle differences and may use the Maxmem use in agp_i810.c as the wrong precedence. Secondly, also mentioned in the = commit log, is the use of realmem in agp.c and then Maxmem in agp_i810.c which = is likely to be confusing. So the change from Maxmem to realmem in = agp_i810.c prioritizes the avoidance of confusion over pedantic correctness, which = we all know isn't achievable anyway. Since agp_i810.c is only for amd64, = i386 and pc98, also stated in the commit log, and all of those have dense = phys. memory, the discrepancy is still within the margin of error. In short: I'm not getting your comments. Do you want me to revert the = change to agp_i810.c so that we can put this to rest? --=20 Marcel Moolenaar marcel@xcllnt.net