Date: Thu, 11 Nov 2010 19:47:31 +0200 From: Andriy Gapon <avg@freebsd.org> To: Alan Cox <alc@rice.edu> Cc: Alan Cox <alc@freebsd.org>, freebsd-current@freebsd.org Subject: Re: minidump size on amd64 Message-ID: <4CDC2C33.1020803@freebsd.org> In-Reply-To: <4CDAE82C.2040708@rice.edu> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4CDC2C33.1020803>