Re: minidump size on amd64

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.