From owner-freebsd-current@FreeBSD.ORG Wed Nov 10 18:45:03 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 4EA62106566B; Wed, 10 Nov 2010 18:45:03 +0000 (UTC) (envelope-from alc@rice.edu) Received: from mh5.mail.rice.edu (mh5.mail.rice.edu [128.42.199.32]) by mx1.freebsd.org (Postfix) with ESMTP id 109908FC12; Wed, 10 Nov 2010 18:45:02 +0000 (UTC) Received: from mh5.mail.rice.edu (localhost.localdomain [127.0.0.1]) by mh5.mail.rice.edu (Postfix) with ESMTP id 5200428F751; Wed, 10 Nov 2010 12:45:02 -0600 (CST) X-Virus-Scanned: by amavis-2.6.4 at mh5.mail.rice.edu, auth channel Received: from mh5.mail.rice.edu ([127.0.0.1]) by mh5.mail.rice.edu (mh5.mail.rice.edu [127.0.0.1]) (amavis, port 10026) with ESMTP id icnvKZ75EoQH; Wed, 10 Nov 2010 12:45:02 -0600 (CST) Received: from adsl-216-63-78-18.dsl.hstntx.swbell.net (adsl-216-63-78-18.dsl.hstntx.swbell.net [216.63.78.18]) (using TLSv1 with cipher RC4-MD5 (128/128 bits)) (No client certificate requested) (Authenticated sender: alc) by mh5.mail.rice.edu (Postfix) with ESMTPSA id CE34328F7A0; Wed, 10 Nov 2010 12:45:01 -0600 (CST) Message-ID: <4CDAE82C.2040708@rice.edu> Date: Wed, 10 Nov 2010 12:45:00 -0600 From: Alan Cox User-Agent: Thunderbird 2.0.0.24 (X11/20100725) MIME-Version: 1.0 To: Andriy Gapon 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> In-Reply-To: <4CD996AF.2070300@freebsd.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailman-Approved-At: Wed, 10 Nov 2010 19:07:02 +0000 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: Wed, 10 Nov 2010 18:45:03 -0000 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; 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; 2. The outermost ()'s aren't needed in the following statement: j = ((va >> PDRSHIFT) & ((1ul << NPDEPGSHIFT) - 1)); 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.