vm_phys_find_freelist_contig is called to search a list of max-sized free page blocks and find one that, when joined with adjacent blocks in memory, can satisfy a request for a memory allocation bigger than any single max-sized free page block. In commit fa8a6585c7522b7de6d29802967bd5eba2f2dcf1, I defined this function in order to offer two improvements: 1) reduce the worst-case search time, and 2) allow solutions that include less-than max-sized free page blocks at the front or back of the giant allocation. However, it turns out that this change introduced an error, reported in In Bug 274592. That error concerns failing to check segment boundaries. This change fixes an error in vm_phys_find_freelist_config that resolves that bug. It also abandons improvement 2), because the value of that improvement is small and because preserving it would require more testing than I am able to do.
Details
The reporter of Bug 274592 reports that, for a test that previously failed about half the time, now 10-12 consecutive tests succeed.
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Don't modify m_ret in the loop, in case iteration happens and that breaks FOREACH. Save the potential return value in m_val instead.
Could you please add some assertions to vm_phys_alloc_contig() to verify that the returned range, if any, satisfies the constraints?
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1363 | ||
| 1381 | or m_ret->phys_addr. | |
| 1391–1393 | ||
| 1403–1405 | There's no reason to expect this condition to be true in general, so "verify" sounds a bit misleading. | |
| 1414–1419 | The loop variable is called m_ret, but it is never returned. | |
| 1416 | Rather than advancing pa for each blocking, translating it to a vm_page, and then checking its order, would it be marginally more efficient to simply advance a vm_page pointer by adding 1 << (VM_NFREEORDER - 1) to it in each iteration? | |
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1397 | This assumes that there is an initialized page structure at m_ret + npages. That is not a valid assumption. You still need "pa" to perform the bounds checks. | |
| sys/vm/vm_phys.c | ||
|---|---|---|
| 960 | This is asking for trouble from someone who doesn't carefully read all of the code. | |
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1402 | This really ought to have a comment saying that it ensures that the loop that follows is only looking at legitimate vm_page structures. | |
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1555 | m_run + npages could be outside the segment containing m_run. | |
As @alc suggests, remove a redundant assertion (aren't they all redundant?), and a pointless NULL check, and a comment which in no way improved performance.
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1395–1400 | Suppose that m_ret is advanced in line 1400, and then that the FOREACH loop iteration ends with 'continue', rather than 'return'. Then the TAILQ_FOREACH advances m to the next item in the list. If there is no m_ret, and m was instead advanced in line 1400, where will the TAILQ_FOREACH take us next? Maybe it'll skip way ahead in the list. Maybe it'll skip back, so that we have an infinite loop. | |
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1466–1467 | ||
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1398 | You're relying on size being greater than max_size. Okay. | |
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1374–1377 | I would KASSERT(size >= max_size, ... | |
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1374–1377 | Actually, strictly greater than. | |
I'm convinced that it's correct. That said, even the person who reported the problem is probably not exercising this while loop:
while (!vm_addr_ok(VM_PAGE_TO_PHYS(m_ret),
size, alignment, boundary) &&
...| sys/vm/vm_phys.c | ||
|---|---|---|
| 1404 | ||
| sys/vm/vm_phys.c | ||
|---|---|---|
| 1387 | ||