riscv pmap: add a demotion assertion
ClosedPublic
Actions

Authored by mhorne on Jan 12 2021, 1:43 AM.

Details

Reviewers

markj

Commits

rG0628f6835702: riscv pmap: add some pv list assertions

Summary

Add a KASSERT to pmap_remove_write() to ensure that the page obtained
from the PV list is not a superpage where we don't expect one. An
identical KASSERT already exists in pmap_clear_modify().

This may help with debugging the panic reported in PR 250866, in which
l3 was found to be NULL. Adding this KASSERT will help narrow down the
cause of this panic the next time it occurs.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36131
Build 33020: arc lint + arc unit

Event Timeline

mhorne created this revision.Jan 12 2021, 1:43 AM

Herald added a subscriber: imp. · View Herald TranscriptJan 12 2021, 1:43 AM

mhorne requested review of this revision.Jan 12 2021, 1:43 AM

Harbormaster completed remote builds in B36131: Diff 82129.Jan 12 2021, 1:43 AM

I think pmap_page_test_mappings() and pmap_page_wired_mappings() could use a similar treatment.

This revision is now accepted and ready to land.Jan 12 2021, 1:49 AM

mhorne added inline comments.Jan 12 2021, 1:50 AM

sys/riscv/riscv/pmap.c
3914	Slightly unrelated, but do you know why the fcmpset/retry loop is required here? `pmap_clear_modify` is structured similarly but performs `pmap_clear_bits` on the PTE instead. Also, can it ever advance without updating `oldl3`?

In D28109#628198, @markj wrote:

I think pmap_page_test_mappings() and pmap_page_wired_mappings() could use a similar treatment.

Sure, I can add those as well.

sys/riscv/riscv/pmap.c
3914	Forget the second question, I double checked the semantics of fcmpset.

markj added inline comments.Jan 12 2021, 2:02 AM

sys/riscv/riscv/pmap.c
3914	I believe it was done this way to handle races with the hardware setting PTE_D. We want to make sure we don't end up clearing PTE_D without observing that it was set. pmap_clear_modify() doesn't care so it can simply clear the bits. atomic_fcmpset_* updates the old value upon failure, in contrast with atomic_cmpset_*.