Various arm64 pmap fixes (and one optimization)
ClosedPublic
Actions

Authored by alc on Jun 10 2019, 5:05 AM.

Details

Reviewers

kib
markj
manu

Commits

rS349003: Change pmap_demote_l2_locked() so that it removes the superpage mapping on a

Summary

Change pmap_demote_l2_locked() so that it removes the superpage mapping on a demotion failure. Otherwise, some callers to pmap_demote_l2_locked(), such as pmap_protect(), may leave an incorrect mapping in place on a demotion failure.

Change pmap_demote_l2_locked() so that it handles addresses that are not superpage aligned. Some callers to pmap_demote_l2_locked(), such as pmap_protect(), may not pass a superpage aligned address.

Change pmap_enter_l2() so that it correctly calls vm_page_free_pages_toq(). The arm64 pmap is updating the count of wired pages when freeing page table pages, so pmap_enter_l2() should pass false to vm_page_free_pages_toq().

Optimize TLB invalidation in pmap_remove_l2().

Test Plan

This patch survived a "make -j4 buildworld" on an Amazon EC2 a1.xlarge (4 cores, 8 GB DRAM) instance.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

alc created this revision.Jun 10 2019, 5:05 AM

Herald added subscribers: emaste, andrew. · View Herald TranscriptJun 10 2019, 5:05 AM

kib accepted this revision.Jun 10 2019, 12:17 PM

This revision is now accepted and ready to land.Jun 10 2019, 12:17 PM

alc added inline comments.Jun 10 2019, 2:52 PM

arm64/arm64/pmap.c
3400	This was essentially a cut-and-paste error in porting pmap_enter_pde() from amd64. To avoid another similar error, I think that a followup change should remove the wired page count updates from the page table page deallocation code so that we would correctly pass "true" here. That would also reduce the number of atomic ops performed.

markj added inline comments.Jun 10 2019, 3:38 PM

arm64/arm64/pmap.c
3400	Note that v_wire_count is now a per-CPU counter, and not updated with atomics.

There is some overlap with D12725.

In D20585#444972, @markj wrote:

There is some overlap with D12725.

I don't think that any of the code overlaps, but the notion of optimizing the TLB invalidation in pmap_remove_l2() was discussed.

andrew added inline comments.Jun 11 2019, 10:14 AM

arm64/arm64/pmap.c
5056	Is this comment correct? `pmap_enter` always sets `ATTR_AF` via `ATTR_DEFAULT`.

alc marked 2 inline comments as done.Jun 11 2019, 3:13 PM

alc added inline comments.

arm64/arm64/pmap.c
5056	No, the comment is incorrect. Thank you for pointing this out. I will update the patch accordingly. As an aside, the functions that create "speculative" mappings, in other words, mappings not created as the direct result of an access, for example, pmap_enter_object() and pmap_enter_quick(), are also using ATTR_DEFAULT and thereby incorrectly marking the pages as referenced.

alc updated this revision to Diff 58524.Jun 11 2019, 3:46 PM

alc marked an inline comment as done.

This revision now requires review to proceed.Jun 11 2019, 3:46 PM

Andrew, as I understand the current state of the pmap code, ATTR_AF is always preset (and never actually cleared) because pmap_fault() does not yet have the ability to set ATTR_AF. Is that correct?

alc edited the test plan for this revision. (Show Details)Jun 11 2019, 8:37 PM

In D20585#445331, @alc wrote:

Andrew, as I understand the current state of the pmap code, ATTR_AF is always preset (and never actually cleared) because pmap_fault() does not yet have the ability to set ATTR_AF. Is that correct?

Correct. It's never made it's way up to the top of things I need to look at.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 12 2019, 8:39 PM

Closed by commit rS349003: Change pmap_demote_l2_locked() so that it removes the superpage mapping on a (authored by alc). · Explain Why

This revision was automatically updated to reflect the committed changes.

Herald added a reviewer: manu. · View Herald TranscriptJun 12 2019, 8:39 PM

Herald added a subscriber: imp. · View Herald Transcript