Tidy up amd64's pmap_copy()

[alc]

Summary

Wrap a long line.

Deindent the innermost loop by making a simple control-flow change.

Add a comment explaining why we may exit the innermost loop early when the page table pages have equal wired counts.

Replace an unnecessary test by a KASSERT. (This is the only functional change.)

[markj]

amd64/amd64/pmap.c

6489 ↗

(On Diff #59226)

I was wondering this in the arm64 review, but I'll ask here: is it actually possible to drop dstmpte's wire count to 0 here given that we added a wiring above?

[kib]

amd64/amd64/pmap.c

6395 ↗

(On Diff #59226)

You fixed the style bug by removing block-local declarations below. Why not fix these too ?

6477 ↗

(On Diff #59226)

How could *dst_pte be non-zero i.e. valid ?

[alc]

amd64/amd64/pmap.c

6489 ↗

(On Diff #59226)

Yes. Suppose that this is our first attempt to create a mapping in dstmpte and we fail.

[alc]

amd64/amd64/pmap.c

6477 ↗

(On Diff #59226)

This is something that we have always done in all of the speculative mapping functions because they are permitted to do nothing. Currently, we don't use this function in a scenario where there should be a preexisting mapping. But, this test acts as a safety belt; if we were to remove this test I would argue that we would need to add code to handle the possibility of a mapping change, e.g., TLB invalidation. IMHO, this safety belt is the better choice.

[alc]

More style fixes.

[kib]

amd64/amd64/pmap.c

6477 ↗

(On Diff #59226)

I mean, if the destination range already contains some ptes filled, and some not (this is probably not even important), then the result of pmap_copy() to such dst is probably useless and even erroneous. My question mean 'should we change the check *dst_pte == 0 to KASSERT(*dst_pte == 0)'.

[alc]

amd64/amd64/pmap.c

6477 ↗

(On Diff #59226)

I would argue that only pmap_copy()'s caller knows whether this test failing is an error or not. So, I would argue against changing this test to a KASSERT but for changing pmap_copy() to return an error when the test fails. In fact, such a change could be made consistently to all of the speculative mapping functions, where they would all return success, failure due to a pre-existing mapping, or failure due to a failed memory allocation. Because we don't currently return a value from any of the speculative mapping functions, the machine-independent layer will sometimes keep iterating, pointlessly attempting more mappings.

[kib]

amd64/amd64/pmap.c

6477 ↗

(On Diff #59226)

I cannot imagine a scenario where it is useful to call pmap_copy() on (partially) filled dst page table.