Page MenuHomeFreeBSD

arm64: Fix pmap_copy()'s handling of 2MB mappings
ClosedPublic

Authored by markj on Jun 4 2021, 6:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 2:58 AM
Unknown Object (File)
Thu, Mar 7, 3:51 PM
Unknown Object (File)
Feb 8 2024, 6:02 AM
Unknown Object (File)
Jan 17 2024, 5:34 PM
Unknown Object (File)
Jan 15 2024, 10:13 AM
Unknown Object (File)
Dec 20 2023, 2:46 AM
Unknown Object (File)
Dec 16 2023, 12:07 AM
Unknown Object (File)
Dec 12 2023, 9:23 PM
Subscribers

Details

Summary

When copying mappings from parent to child, we clear the accessed and
dirty bits. This is done for both 4KB and 2MB PTEs. However,
pmap_demote_l2() asserts that writable superpages must be dirty. This
is to avoid races with the MMU setting the dirty bit during promotion
and demotion. pmap_copy() can create clean, writable superpage
mappings, so it violates this assertion.

Modify pmap_copy() to make new 2MB mappings read-only, like we do on
amd64. I am not sure though why we shouldn't simply copy the dirty bit
over to the child.

Fixes: ca2cae0b4dd
Reported by: Jenkins via mhorne

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Jun 4 2021, 6:22 PM
kib accepted this revision.EditedJun 4 2021, 8:24 PM

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

This revision is now accepted and ready to land.Jun 4 2021, 8:24 PM
In D30643#688425, @kib wrote:

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

When writing the dirty pages, won't we downgrade all writable mappings to RO and clear dirty bits from the copied mapping as well?

In D30643#688425, @kib wrote:

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

When writing the dirty pages, won't we downgrade all writable mappings to RO and clear dirty bits from the copied mapping as well?

But then system would need to demote and invalidate TLB. I still think that not marking mapping as dirty is the right thing to do, when we know that there were no write.

You wrote, "Modify pmap_copy() to make new 2MB mappings read-only, like we do on amd64. I am not sure though why we shouldn't simply copy the dirty bit over to the child."

I'm confused. :-) I'm looking at the amd64 source code right now, and for 2MB page mappings it copies PG_A and PG_M. Moreover, it copies PG_RW. However, for 4KB page mappings, it does clear PG_A and PG_M. In other words, we don't handle different page sizes uniformly.

In D30643#688707, @alc wrote:

You wrote, "Modify pmap_copy() to make new 2MB mappings read-only, like we do on amd64. I am not sure though why we shouldn't simply copy the dirty bit over to the child."

I'm confused. :-) I'm looking at the amd64 source code right now, and for 2MB page mappings it copies PG_A and PG_M. Moreover, it copies PG_RW. However, for 4KB page mappings, it does clear PG_A and PG_M. In other words, we don't handle different page sizes uniformly.

I see, I switched them around in my head when reading the code. The amd64 behaviour makes more sense to me now.

Preserve AF, DBM and RW flags when copying 2MB pages.

This revision now requires review to proceed.Jun 6 2021, 4:23 PM
This revision is now accepted and ready to land.Jun 6 2021, 6:20 PM

I still think it is better to clear PG_A|PG_M|PG_RW on copy of 2M pages for amd64. Ideally we would only clear PG_A|PG_M, but this breaks known invariant.

In D30643#688425, @kib wrote:

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

No. To write the page(s) out, we would first perform a pmap_remove_write(), which clears PG_M from all of the mappings, including the copied ones.