Re-count available PV entries after allocating a new chunk.

[markj]

Summary

The call to reclaim_pv_chunk() may destroy a PV chunk with free entries
for the current pmap, and we were not accounting for this in "avail".
To keep reserve_pv_entries() simple, just unconditionally count
available entries after allocating a new chunk.

Test Plan

Peter is validating the patch. The bug was found while reading code.

[kib]

This should be fine.

Can we return an indicator if the page freed from the locked pmap, and retry only in this case ?

[markj]

In D15911#336659, @kib wrote:

This should be fine.

Can we return an indicator if the page freed from the locked pmap, and retry only in this case ?

We can. I implemented it this way originally, but found it a bit ugly, and strictly speaking we only need to retry if the chunk was freed from the locked pmap *and* it contained free entries. That is, even with your suggestion we may still retry when it is not necessary. If you prefer that approach I'll implement it instead, but I mildly prefer this patch because it's simpler than the alternative.

[kib]

In D15911#336740, @markj wrote:

In D15911#336659, @kib wrote:

This should be fine.

Can we return an indicator if the page freed from the locked pmap, and retry only in this case ?

We can. I implemented it this way originally, but found it a bit ugly, and strictly speaking we only need to retry if the chunk was freed from the locked pmap *and* it contained free entries. That is, even with your suggestion we may still retry when it is not necessary. If you prefer that approach I'll implement it instead, but I mildly prefer this patch because it's simpler than the alternative.

Sure I do not object against adding the additional check for the chunk to contain a free entry, and it is cheap. My motivation for the suggestion is that we should not penalize the situation when get_pv_entry() failed to allocate a page, too much.

But I do not object to the patch in its current form.

[alc]

In D15911#336915, @kib wrote:

In D15911#336740, @markj wrote:

In D15911#336659, @kib wrote:

This should be fine.

Can we return an indicator if the page freed from the locked pmap, and retry only in this case ?

We can. I implemented it this way originally, but found it a bit ugly, and strictly speaking we only need to retry if the chunk was freed from the locked pmap *and* it contained free entries. That is, even with your suggestion we may still retry when it is not necessary. If you prefer that approach I'll implement it instead, but I mildly prefer this patch because it's simpler than the alternative.

Sure I do not object against adding the additional check for the chunk to contain a free entry, and it is cheap. My motivation for the suggestion is that we should not penalize the situation when get_pv_entry() failed to allocate a page, too much.

But I do not object to the patch in its current form.

I would "split the difference" between these two approaches. By far, the expected case is that vm_page_alloc() succeeds. In fact, I'll make up a number. I expect it to succeed 99.99% of the time. And, when it succeeds, it's pointless to retry. Please limit the restarts to any time that reclaim_pv_chunk() is called to obtain the page, but not worry about where exactly the page came from. This case happens so infrequently that it's not worth the complexity of determining whether the page came from the current pmap.

[markj]

• Only recount after a reclaim.

[markj]

I note that arm64 has this bug as well. I'll fix that identically if the current diff is ok.

[alc]

sys/amd64/amd64/pmap.c

3587 ↗

(On Diff #44238)

You could actually initialize reclaimed to false outside the loop and reset it to false when performing the goto. Because dump_add_page() is a function call, the variable reclaimed will have to be in a callee-saves register or on the stack even with its shorter life-time in the current version.

[markj]

sys/amd64/amd64/pmap.c

3587 ↗

(On Diff #44238)

Why set it to false before the goto as well?

[alc]

sys/amd64/amd64/pmap.c

3587 ↗

(On Diff #44238)

There is none. :-)

[markj]

• Only set "reclaimed" once when executing the loop.
• Fix the problem in the arm64 pmap code.

[alc]

In D15911#337606, @markj wrote:

I note that arm64 has this bug as well. I'll fix that identically if the current diff is ok.

Although 32-bit arm and i386 have essentially the same PV management code, they don't have this bug, because they don't have the optimization in which the bug exists.