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.
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
