Changeset View
Standalone View
vm_reserv.c
Show First 20 Lines • Show All 1,165 Lines • ▼ Show 20 Lines | vm_reserv_reclaim_inactive(int domain) | ||||
return (FALSE); | return (FALSE); | ||||
} | } | ||||
/* | /* | ||||
* Determine whether this reservation has free pages that satisfy the given | * Determine whether this reservation has free pages that satisfy the given | ||||
* request for contiguous physical memory. Start searching from the lower | * request for contiguous physical memory. Start searching from the lower | ||||
* bound, defined by low_index. | * bound, defined by low_index. | ||||
* | * | ||||
* The free page queue lock must be held. | * The reservation must be locked. | ||||
alc: This comment is no longer correct. The requirement is that the reservation is locked. See the… | |||||
*/ | */ | ||||
static bool | static bool | ||||
vm_reserv_test_contig(vm_reserv_t rv, u_long npages, vm_paddr_t low, | vm_reserv_test_contig(vm_reserv_t rv, u_long npages, vm_paddr_t low, | ||||
vm_paddr_t high, u_long alignment, vm_paddr_t boundary) | vm_paddr_t high, u_long alignment, vm_paddr_t boundary) | ||||
{ | { | ||||
vm_paddr_t pa, size; | vm_paddr_t pa, size; | ||||
u_long changes; | u_long changes, hi, lo; | ||||
alcUnsubmitted Done Inline ActionsThere exists at least one platform, i386 PAE, where vm_paddr_t is wider than u_long, so simply changing "hi" and "lo" to u_long won't address the hypothetical overflow problems. (In practice, there would never be a problem because i386 PAE won't use all the bits available in vm_paddr_t, but a static analysis tool, like Coverity, is not going to know that.) alc: There exists at least one platform, i386 PAE, where vm_paddr_t is wider than u_long, so simply… | |||||
kibUnsubmitted Done Inline ActionsI believe that 32bit powerpc also got a similar mode recently. kib: I believe that 32bit powerpc also got a similar mode recently. | |||||
int bitpos, bits_left, i, hi, lo, n; | int bitpos, bits_left, i, n; | ||||
vm_reserv_assert_locked(rv); | vm_reserv_assert_locked(rv); | ||||
alcUnsubmitted Done Inline ActionsI would also assert that "npages < VM_LEVEL_0_NPAGES". alc: I would also assert that "npages < VM_LEVEL_0_NPAGES". | |||||
size = npages << PAGE_SHIFT; | size = npages << PAGE_SHIFT; | ||||
pa = VM_PAGE_TO_PHYS(&rv->pages[0]); | pa = VM_PAGE_TO_PHYS(&rv->pages[0]); | ||||
KASSERT(pa + VM_LEVEL_0_SIZE - size >= low, | |||||
("%s: reservation is too low", __func__)); | |||||
lo = (pa < low) ? | lo = (pa < low) ? | ||||
((low + PAGE_MASK - pa) >> PAGE_SHIFT) : 0; | ((low + PAGE_MASK - pa) >> PAGE_SHIFT) : 0; | ||||
i = lo / NBPOPMAP; | i = lo / NBPOPMAP; | ||||
changes = rv->popmap[i] | ((1UL << (lo % NBPOPMAP)) - 1); | changes = rv->popmap[i] | ((1UL << (lo % NBPOPMAP)) - 1); | ||||
KASSERT(pa + size <= high, | |||||
("%s: reservation is too high", __func__)); | |||||
hi = (pa + VM_LEVEL_0_SIZE > high) ? | hi = (pa + VM_LEVEL_0_SIZE > high) ? | ||||
((high + PAGE_MASK - pa) >> PAGE_SHIFT) : VM_LEVEL_0_NPAGES; | ((high - pa) >> PAGE_SHIFT) : VM_LEVEL_0_NPAGES; | ||||
Done Inline ActionsI question the "+ PAGE_MASK" here. In other words, I suspect that "high" should be truncated, not rounded. alc: I question the "+ PAGE_MASK" here. In other words, I suspect that "high" should be truncated… | |||||
n = hi / NBPOPMAP; | n = hi / NBPOPMAP; | ||||
bits_left = hi % NBPOPMAP; | bits_left = hi % NBPOPMAP; | ||||
hi = lo = -1; | hi = lo = -1; | ||||
for (;;) { | for (;;) { | ||||
/* | /* | ||||
* "changes" is a bitmask that marks where a new sequence of | * "changes" is a bitmask that marks where a new sequence of | ||||
* 0s or 1s begins in popmap[i], with last bit in popmap[i-1] | * 0s or 1s begins in popmap[i], with last bit in popmap[i-1] | ||||
* considered to be 1 if and only if lo == hi. The bits of | * considered to be 1 if and only if lo == hi. The bits of | ||||
Show All 11 Lines | while (changes != 0) { | ||||
if (lo == hi) { | if (lo == hi) { | ||||
lo = NBPOPMAP * i + bitpos; | lo = NBPOPMAP * i + bitpos; | ||||
continue; | continue; | ||||
} | } | ||||
hi = NBPOPMAP * i + bitpos; | hi = NBPOPMAP * i + bitpos; | ||||
pa = VM_PAGE_TO_PHYS(&rv->pages[lo]); | pa = VM_PAGE_TO_PHYS(&rv->pages[lo]); | ||||
if ((pa & (alignment - 1)) != 0) { | if ((pa & (alignment - 1)) != 0) { | ||||
/* Skip to next aligned page. */ | /* Skip to next aligned page. */ | ||||
lo += (((pa - 1) | (alignment - 1)) + 1) >> | lo += (((pa - 1) | (alignment - 1)) + 1) >> | ||||
PAGE_SHIFT; | PAGE_SHIFT; | ||||
Done Inline ActionsAn absurdly large alignment could cause integer overflow here as well. alc: An absurdly large alignment could cause integer overflow here as well. | |||||
if (lo >= VM_LEVEL_0_NPAGES) | if (lo >= VM_LEVEL_0_NPAGES) | ||||
return (false); | return (false); | ||||
pa = VM_PAGE_TO_PHYS(&rv->pages[lo]); | pa = VM_PAGE_TO_PHYS(&rv->pages[lo]); | ||||
} | } | ||||
if (((pa ^ (pa + size - 1)) & ~(boundary - 1)) != 0) { | if (((pa ^ (pa + size - 1)) & ~(boundary - 1)) != 0) { | ||||
/* Skip to next boundary-matching page. */ | /* Skip to next boundary-matching page. */ | ||||
lo += (((pa - 1) | (boundary - 1)) + 1) >> | lo += (((pa - 1) | (boundary - 1)) + 1) >> | ||||
PAGE_SHIFT; | PAGE_SHIFT; | ||||
Done Inline ActionsAn absurdly large boundary could cause integer overflow here as well. alc: An absurdly large boundary could cause integer overflow here as well. | |||||
if (lo >= VM_LEVEL_0_NPAGES) | if (lo >= VM_LEVEL_0_NPAGES) | ||||
return (false); | return (false); | ||||
pa = VM_PAGE_TO_PHYS(&rv->pages[lo]); | pa = VM_PAGE_TO_PHYS(&rv->pages[lo]); | ||||
} | } | ||||
if (lo * PAGE_SIZE + size <= hi * PAGE_SIZE) | if (lo < hi && npages <= hi - lo) | ||||
Done Inline ActionsIf 'lo' and 'hi' stayed as 'int', coverity would still complain due to 'npages' being u_long, but since lo and hi are vm_paddr_t, it won't complain. I do think that 'lo + npages <= hi' is actually more readable. You might consider leaving 'lo' and 'hi' as 'int' (or even 'u_int') and changing 'npages' to 'u_int' instead of u_long. It would seem that 'npages' is confined to the range (0, VM_LEVEL_0_NPAGES]. jhb: If 'lo' and 'hi' stayed as 'int', coverity would still complain due to 'npages' being u_long… | |||||
return (true); | return (true); | ||||
lo = hi; | lo = hi; | ||||
} | } | ||||
if (++i < n) | if (++i < n) | ||||
changes = rv->popmap[i]; | changes = rv->popmap[i]; | ||||
else if (i == n) | else if (i == n) | ||||
changes = bits_left == 0 ? -1UL : | changes = bits_left == 0 ? -1UL : | ||||
(rv->popmap[n] | (-1UL << bits_left)); | (rv->popmap[n] | (-1UL << bits_left)); | ||||
else | else | ||||
return (false); | return (false); | ||||
} | } | ||||
} | } | ||||
/* | /* | ||||
* Searches the partially populated reservation queue for the least recently | * Searches the partially populated reservation queue for the least recently | ||||
* changed reservation with free pages that satisfy the given request for | * changed reservation with free pages that satisfy the given request for | ||||
* contiguous physical memory. If a satisfactory reservation is found, it is | * contiguous physical memory. If a satisfactory reservation is found, it is | ||||
* broken. Returns true if a reservation is broken and false otherwise. | * broken. Returns true if a reservation is broken and false otherwise. | ||||
* | |||||
* The free page queue lock must be held. | |||||
*/ | */ | ||||
Done Inline ActionsNot here. Only in vm_reserv_test_contig(). This function acquires the reservation lock for vm_reserv_test_contig(). (And, to be clear, the original comment here "The free page queue lock must be held." is no longer correct and can be removed.) alc: Not here. Only in vm_reserv_test_contig(). This function acquires the reservation lock for… | |||||
boolean_t | boolean_t | ||||
vm_reserv_reclaim_contig(int domain, u_long npages, vm_paddr_t low, | vm_reserv_reclaim_contig(int domain, u_long npages, vm_paddr_t low, | ||||
vm_paddr_t high, u_long alignment, vm_paddr_t boundary) | vm_paddr_t high, u_long alignment, vm_paddr_t boundary) | ||||
{ | { | ||||
vm_paddr_t pa, size; | vm_paddr_t pa, size; | ||||
vm_reserv_t rv, rvn; | vm_reserv_t rv, rvn; | ||||
if (npages > VM_LEVEL_0_NPAGES - 1) | if (npages > VM_LEVEL_0_NPAGES - 1) | ||||
return (false); | return (FALSE); | ||||
Done Inline ActionsSince this function's return type is still boolean_t, the spelling of false should have remained "FALSE". alc: Since this function's return type is still boolean_t, the spelling of false should have… | |||||
size = npages << PAGE_SHIFT; | size = npages << PAGE_SHIFT; | ||||
vm_reserv_domain_lock(domain); | vm_reserv_domain_lock(domain); | ||||
again: | again: | ||||
for (rv = TAILQ_FIRST(&vm_rvq_partpop[domain]); rv != NULL; rv = rvn) { | for (rv = TAILQ_FIRST(&vm_rvq_partpop[domain]); rv != NULL; rv = rvn) { | ||||
rvn = TAILQ_NEXT(rv, partpopq); | rvn = TAILQ_NEXT(rv, partpopq); | ||||
pa = VM_PAGE_TO_PHYS(&rv->pages[0]); | pa = VM_PAGE_TO_PHYS(&rv->pages[0]); | ||||
if (pa + VM_LEVEL_0_SIZE - size < low) { | if (pa + VM_LEVEL_0_SIZE - size < low) { | ||||
/* This entire reservation is too low; go to next. */ | /* This entire reservation is too low; go to next. */ | ||||
Show All 13 Lines | if (vm_reserv_trylock(rv) == 0) { | ||||
continue; | continue; | ||||
} | } | ||||
} else | } else | ||||
vm_reserv_domain_unlock(domain); | vm_reserv_domain_unlock(domain); | ||||
if (vm_reserv_test_contig(rv, npages, low, high, | if (vm_reserv_test_contig(rv, npages, low, high, | ||||
alignment, boundary)) { | alignment, boundary)) { | ||||
vm_reserv_reclaim(rv); | vm_reserv_reclaim(rv); | ||||
vm_reserv_unlock(rv); | vm_reserv_unlock(rv); | ||||
return (true); | return (TRUE); | ||||
} | } | ||||
vm_reserv_unlock(rv); | vm_reserv_unlock(rv); | ||||
vm_reserv_domain_lock(domain); | vm_reserv_domain_lock(domain); | ||||
if (rvn != NULL && !rvn->inpartpopq) | if (rvn != NULL && !rvn->inpartpopq) | ||||
goto again; | goto again; | ||||
} | } | ||||
vm_reserv_domain_unlock(domain); | vm_reserv_domain_unlock(domain); | ||||
return (false); | return (FALSE); | ||||
} | } | ||||
/* | /* | ||||
* Transfers the reservation underlying the given page to a new object. | * Transfers the reservation underlying the given page to a new object. | ||||
* | * | ||||
* The object must be locked. | * The object must be locked. | ||||
*/ | */ | ||||
void | void | ||||
▲ Show 20 Lines • Show All 113 Lines • Show Last 20 Lines |
This comment is no longer correct. The requirement is that the reservation is locked. See the below assertion.