Re: minidump size on amd64

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