Page MenuHomeFreeBSD
Differential D8323

Handle pmap_enter() over an existing 4/2M page on i386.
ClosedPublic
Actions

Authored by kib on Oct 22 2016, 10:40 AM.
Tags
None
Referenced Files
F106070327: D8323.id21614.diff
Tue, Dec 24, 9:42 PM
F106054837: D8323.id21676.diff
Tue, Dec 24, 2:57 PM
F106048735: D8323.id21673.diff
Tue, Dec 24, 12:01 PM
Unknown Object (File)
Fri, Dec 20, 12:17 PM
Unknown Object (File)
Fri, Dec 20, 5:42 AM
Unknown Object (File)
Thu, Dec 5, 6:27 PM
Unknown Object (File)
Sat, Nov 30, 7:12 PM
Unknown Object (File)
Fri, Nov 29, 7:22 PM
View All Files
Subscribers
imp
pho

Details

Reviewers
alc
markj
Commits
rS308029: Handle pmap_enter() over an existing 4/2M page in KVA on i386.
Summary

This change apparently sit in my local tree for long time.

I do not see why it is impossible for e.g. two threads to fault on the same address simultaneously, then one thread creates the mapping in pmap_enter() and promotes it, and only then the second thread enters pmap_enter(), either due to locking or locking and scheduling. In this case, amd64 pmap demotes large page first, while i386 panics for some contrast.

Handle the i386 pmap_enter() same as amd64, demote.

Test Plan

I believe it was done after some actual Peter report of 'panic("pmap_enter: attempted pmap_enter on 4MB page")', but the situation is very hard to reproduce.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib updated this revision to Diff 21614.Oct 22 2016, 10:40 AM
kib retitled this revision from to Handle pmap_enter() over an existing 4/2M page on i386..
kib updated this object.
kib edited the test plan for this revision. (Show Details)
kib added reviewers: alc, markj.
kib set the repository for this revision to rS FreeBSD src repository - subversion.
kib added a subscriber: pho.
Herald added a subscriber: imp. · View Herald TranscriptOct 22 2016, 10:40 AM
alc edited edge metadata.Oct 22 2016, 5:19 PM

Unless the mapping is a kernel virtual address, the demotion is already being performed at the start of pmap_allocpte().

kib added a comment.Oct 23 2016, 1:32 AM
In D8323#173239, @alc wrote:

Unless the mapping is a kernel virtual address, the demotion is already being performed at the start of pmap_allocpte().

Yes. It is probably practically impossible to have a promotion on the kernel (sub)map entry handled by pmap_enter() on i386.

But still, I would like to either add the demotion call for KVA just for completeness, or a comment noting what you said + note to the panic that it is for kernel addresses. What do you prefer ?

alc added a comment.Oct 24 2016, 10:17 PM
In D8323#173301, @kib wrote:
In D8323#173239, @alc wrote:

Unless the mapping is a kernel virtual address, the demotion is already being performed at the start of pmap_allocpte().

Yes. It is probably practically impossible to have a promotion on the kernel (sub)map entry handled by pmap_enter() on i386.

But still, I would like to either add the demotion call for KVA just for completeness, or a comment noting what you said + note to the panic that it is for kernel addresses. What do you prefer ?

Let's do the former. I don't see a downside to making it work for KVA.

sys/i386/i386/pmap.c
3472 ↗(On Diff #21614)

If pmap_demote_pde() can't allocate a page table page (and returns FALSE), then we need to execute the "else if". Take a look at how I wrote this on amd64 with the demotion call included in the test expression of the "if". You need to do the same here.

kib added a comment.Oct 25 2016, 10:09 AM

For KVA, pde must exist and be valid, and demotion failure is impossible, due to pmap_growkernel() preallocating the page table page, am I wrong ? If not, I propose to leave the UVA case as is, and just call pmap_demote_pde() for KVA.

kib updated this revision to Diff 21666.Oct 25 2016, 10:10 AM
kib edited edge metadata.

Only add code for KVA demotion.

alc accepted this revision.Oct 25 2016, 5:27 PM
alc edited edge metadata.
In D8323#173555, @kib wrote:

For KVA, pde must exist and be valid, and demotion failure is impossible, due to pmap_growkernel() preallocating the page table page, am I wrong ? If not, I propose to leave the UVA case as is, and just call pmap_demote_pde() for KVA.

You are correct.

sys/i386/i386/pmap.c
3488 ↗(On Diff #21666)

"va is for KVA, so pmap_demote_pde() will never fail to install a page table page.

This revision is now accepted and ready to land.Oct 25 2016, 5:27 PM
alc added inline comments.Oct 25 2016, 5:29 PM
sys/i386/i386/pmap.c
3500 ↗(On Diff #21666)

As an aside to this change, the above comment makes no sense. The page table page allocation has already occurred.

kib updated this revision to Diff 21673.Oct 25 2016, 5:54 PM
kib edited edge metadata.

Update comments.

This revision now requires review to proceed.Oct 25 2016, 5:54 PM
alc added inline comments.Oct 25 2016, 6:07 PM
sys/i386/i386/pmap.c
3501 ↗(On Diff #21673)

You use "should" in the next sentence, and these two sentences are related. I would use "should" here as well.

3502 ↗(On Diff #21673)

"... should have either ..."

3503 ↗(On Diff #21673)

You don't need the "," here.

alc added inline comments.Oct 25 2016, 6:11 PM
sys/i386/i386/pmap.c
3503 ↗(On Diff #21673)

Also, "... demoted the existing ..."

kib updated this revision to Diff 21676.Oct 25 2016, 6:24 PM
kib edited edge metadata.

Grammar fixes.

alc accepted this revision.Oct 25 2016, 6:32 PM
alc edited edge metadata.
This revision is now accepted and ready to land.Oct 25 2016, 6:32 PM
markj accepted this revision.Oct 26 2016, 9:10 PM
markj edited edge metadata.
Closed by commit rS308029: Handle pmap_enter() over an existing 4/2M page in KVA on i386. (authored by kib). · Explain WhyOct 28 2016, 11:53 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
head/
sys/
i386/
i386/
32 lines

Diff 21760

View Options

head/sys/i386/i386/pmap.c

Loading...