vm_phys: fix freelist_contig

[dougm]

Summary

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.

Test Plan

The reporter of Bug 274592 reports that, for a test that previously failed about half the time, now 10-12 consecutive tests succeed.

[dougm]

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.

[markj]

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?

[dougm]

Apply suggestions from @markj.

[dougm]

Add a missing assignment.

[dougm]

Re-enter an iterator for checking empty blocks.

[alc]

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.

[alc]

sys/vm/vm_phys.c

960

This is asking for trouble from someone who doesn't carefully read all of the code.

[alc]

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.

[dougm]

Address feedback from @alc.

[markj]

sys/vm/vm_phys.c

1558

m_run + npages could be outside the segment containing m_run.

[dougm]

Fix an assertion identified by @markj.

[alc]

We may want to issue this as an errata for 14.0, so can we have a minimal version without style changes?

sys/vm/vm_phys.c

1558

1560

Use m_run->segind instead to compute this faster.

1563

[dougm]

Apply @alc suggestions. Remove some inessential parts.

[alc]

sys/vm/vm_phys.c

1533

This assertion is redundant. We know that the domain is locked, and we are still operating on the same domain.

1561

The NULL check is now pointless.

[dougm]

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.

[alc]

sys/vm/vm_phys.c

1395

I don't see the point of introducing a new variable, m_ret,

1416

The semi-colon at the end of the line is too easy to overlook.

[dougm]

sys/vm/vm_phys.c

1395

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.

[dougm]

For @alc, change a semicolon to empty braces.

[alc]

sys/vm/vm_phys.c

1466–1467

[dougm]

Fix a comment, suggested by @alc.

[alc]

sys/vm/vm_phys.c

1363

This comment is not currently very descriptive of what this function does.

1379–1380

This describes what the code does, but not why. Specifically, why do we care that the preceding block is free?

1398

I don't see how 'size' is correct here.

[alc]

sys/vm/vm_phys.c

1398

You're relying on size being greater than max_size. Okay.

[alc]

sys/vm/vm_phys.c

1374–1377

I would KASSERT(size >= max_size, ...

[alc]

sys/vm/vm_phys.c

1374–1377

Actually, strictly greater than.

[alc]

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

[dougm]

Add assertion. Add/update some comments.

[alc]

sys/vm/vm_phys.c

1387

[dougm]

Edit a couple of comments.

[markj]

sys/vm/vm_phys.c

1407–1408

Is this check inverted? Certainly the ! looks wrong.

1564

[dougm]

Fix typos identified by @markj.