Page MenuHomeFreeBSD

Use regular stores for updating PTEs.
ClosedPublic

Authored by markj on Jan 2 2019, 6:16 PM.

Details

Summary

We currently use an atomic_swap to atomically store and load a PTE, but
this is excessive. Use regular loads and stores instead when possible.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Jan 2 2019, 6:16 PM
kib accepted this revision.Jan 2 2019, 9:46 PM
kib added inline comments.
sys/riscv/riscv/pmap.c
1901 ↗(On Diff #52485)

Could this be expressed by in-place and-ing instead of load/and/store ?

This revision is now accepted and ready to land.Jan 2 2019, 9:46 PM
jhb added inline comments.Jan 2 2019, 11:50 PM
sys/riscv/riscv/pmap.c
1901 ↗(On Diff #52485)

I think it's more consistent with the current code. However, if we wanted to drop pmap_load/pmap_store entirely and just use regular assignments for simple loads and stores, then replacing this with 'entry &= ~PTE_W' probably would make sense. amd64 uses regular assignments ('*pte = newpte;', etc.) It's not clear to me that the wrappers are useful if you don't really need them.

markj added inline comments.Jan 3 2019, 12:21 AM
sys/riscv/riscv/pmap.c
1901 ↗(On Diff #52485)

(Note that this code is changed in D18719.)

I somewhat prefer to use the wrappers since they guard accesses of memory that may be updated concurrently by hardware. In practice they probably aren't necessary; it's hard to see any cases in the amd64 pmap where a compiler optimization might introduce an unexpected side effect, but I don't see why it isn't possible.

This revision was automatically updated to reflect the committed changes.