From owner-freebsd-current@FreeBSD.ORG Thu Nov 11 17:47:44 2010 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 28F4B1065673; Thu, 11 Nov 2010 17:47:44 +0000 (UTC) (envelope-from avg@freebsd.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 19F608FC17; Thu, 11 Nov 2010 17:47:42 +0000 (UTC) Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua [212.40.38.101]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id TAA16320; Thu, 11 Nov 2010 19:47:31 +0200 (EET) (envelope-from avg@freebsd.org) Message-ID: <4CDC2C33.1020803@freebsd.org> Date: Thu, 11 Nov 2010 19:47:31 +0200 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.9.2.12) Gecko/20101029 Lightning/1.0b2 Thunderbird/3.1.6 MIME-Version: 1.0 To: Alan Cox References: <4CA0DA49.2090006@freebsd.org> <4CA3A48A.5070300@freebsd.org> <4CA3BD1E.5070807@rice.edu> <4CA5911E.3000101@freebsd.org> <4CAE0060.7050607@freebsd.org> <4CAECC4D.90707@rice.edu> <4CD1AA45.7000504@freebsd.org> <4CD1AD80.2090903@rice.edu> <4CD1D4AA.3060309@freebsd.org> <4CD8FFFF.3070106@rice.edu> <4CD996AF.2070300@freebsd.org> <4CDAE82C.2040708@rice.edu> In-Reply-To: <4CDAE82C.2040708@rice.edu> X-Enigmail-Version: 1.1.2 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: Alan Cox , freebsd-current@freebsd.org Subject: Re: minidump size on amd64 X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Nov 2010 17:47:44 -0000 on 10/11/2010 20:45 Alan Cox said the following: > Andriy Gapon wrote: >> on 09/11/2010 10:02 Alan Cox said the following: >> >>> The kernel portion of the patch looks correct. If I were to make one stylistic >>> suggestion, it would be to make the control flow of the outer and inner loops as >>> similar as possible, that is, >>> >>> for (... >>> if ((pdp[i] & PG_V) == 0) { >>> ... >>> continue; >>> } >>> if ((pdp[i] & PG_PS) != 0) { >>> ... >>> continue; >>> } >>> for (... >>> if ((pd[j] & PG_V) == 0) >>> continue; >>> if ((pd[j] & PG_PS) != 0) { >>> ... >>> continue; >>> } >>> for (... >>> if ((pt[x] & PG_V) == 0) >>> continue; >>> ... >>> >>> I think this would make the code a little easier to follow. >>> >> >> This is a very nice suggestion, thank you. >> Besides the uniformity some horizontal space is saved too :-) >> Updated patch (only kernel part) is here: >> http://people.freebsd.org/~avg/amd64-minidump.5.diff >> >> > > In the later loop, where you actually write the page directory pages, the "va += > ..." in the following looks like a bug because you also update "va" in for (...): > > + > + /* 1GB page is represented as 512 2MB pages in a dump */ > + if ((pdp[i] & PG_PS) != 0) { > + va += NBPDP; Yes, thank you - a copy/paste bug. > My last three comments are: > > 1. I would move the assignment > > i = (va >> PDPSHIFT) & ((1ul << NPDPEPGSHIFT) - 1); > > > so that it comes after > > pmapsize += PAGE_SIZE; OK. > 2. The outermost ()'s aren't needed in the following statement: > > j = ((va >> PDRSHIFT) & ((1ul << NPDEPGSHIFT) - 1)); > Yes. > 3. I would suggest rewriting the following: > > + pa = pdp[i] & PG_PS_FRAME; > + for (j = 0; j < NPDEPG; j++) > + fakepd[j] = (pa + (j << PDRSHIFT)) | > + PG_V | PG_PS | PG_RW | PG_A | PG_M; > > > > fakepd[0] = pdp[i]; > for (j = 1; j < NPDEPG; j++) > fakepd[j] = fakepd[j - 1] + NBPDR; > > > Then, whatever properties the pdp entry has will be inherited by the pd entry. Very nice, thank you! I overlooked the fact that "super" PDPE and "super" PDE have identical layout. I am going to commit this code tonight after applying the above changes. Thank you very much for all the guidance and help! -- Andriy Gapon