Page MenuHomeFreeBSD
Differential D17184

amd64: ifunc-ify pmap_invalidate_XXX() TLB invalidation functions.
ClosedPublic
Actions

Authored by kib on Sep 16 2018, 10:00 PM.
Tags
None
Referenced Files
F106097082: D17184.id48323.diff
Wed, Dec 25, 8:43 AM
Unknown Object (File)
Oct 27 2024, 5:36 AM
Unknown Object (File)
Oct 24 2024, 1:48 PM
Unknown Object (File)
Oct 24 2024, 1:47 PM
Unknown Object (File)
Oct 24 2024, 1:47 PM
Unknown Object (File)
Oct 24 2024, 1:47 PM
Unknown Object (File)
Oct 24 2024, 1:47 PM
Unknown Object (File)
Oct 24 2024, 1:28 PM
View All 66 Files
Subscribers
emaste
imp
pho

Details

Reviewers
alc
markj
jhb
Commits
rS338862: Convert x86 TLB top-level invalidation functions to ifuncs.
Summary

This is a continuation of D17181.

Diff Detail

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

Event Timeline

kib created this revision.Sep 16 2018, 10:00 PM
Herald added a subscriber: imp. · View Herald TranscriptSep 16 2018, 10:00 PM
Harbormaster completed remote builds in B19611: Diff 48089.Sep 16 2018, 10:00 PM
kib added a comment.Sep 16 2018, 10:01 PM

This diff is generated from the branch where pmap_activate_sw() work was done. I will update the review after the context switch chunks will be committed.

kib updated this revision to Diff 48122.Sep 17 2018, 3:55 PM

Distilled patch after pmap_activate_sw() changes were committed.

Harbormaster completed remote builds in B19624: Diff 48122.Sep 17 2018, 3:56 PM
markj accepted this revision.Sep 17 2018, 4:10 PM
This revision is now accepted and ready to land.Sep 17 2018, 4:10 PM
kib added a subscriber: pho.Sep 20 2018, 8:25 PM

Peter, could you, please, give some run time to this patch ? It is only relevant for amd64, but in two modes, with pcid enabled and with vm.pmap.pcid_enabled=0. In both cases, some threading loads and some mmap tests are enough.

pho added a comment.Sep 21 2018, 6:55 AM
In D17184#367884, @kib wrote:

Peter, could you, please, give some run time to this patch ? It is only relevant for amd64, but in two modes, with pcid enabled and with vm.pmap.pcid_enabled=0. In both cases, some threading loads and some mmap tests are enough.

Did that + a buildworld / installworld. No problems seen.

alc accepted this revision.Sep 21 2018, 4:53 PM
alc added inline comments.
sys/amd64/amd64/pmap.c
1718 ↗(On Diff #48122)

I would suggest refactoring these sentences:

"Because pm_pcid is recalculated on a context switch, we must disable switching. Otherwise, we might use a stale value below."

1722 ↗(On Diff #48122)

Arguably, the blank line isn't needed here because the comment above is relevant to this entire block.

1745–1752 ↗(On Diff #48122)

This has been deindented by one level. It might be worth reflowing the paragraph.

1801–1802 ↗(On Diff #48122)

This is an aside. I wonder how often these days we wake up a halted processor to perform a shootdown on the kernel address space.

2020–2021 ↗(On Diff #48122)

Is there any reason for this particular ordering of these statements? Swapping their order, would mean that the variable mask did not have to be live across a function call, and thus the compiler would not have to use a callee-saves register.

kib marked 5 inline comments as done.Sep 21 2018, 5:33 PM
kib added inline comments.
sys/amd64/amd64/pmap.c
1745–1752 ↗(On Diff #48122)

My emacs does not want to change the formatting.

1801–1802 ↗(On Diff #48122)

Yes. I read some descriptions (but not the code) that Linux does not IPI CPUs which are in deep sleep, say C2 or C3. Instead, a flag is set and CPU does TLB invalidation on wakeup.

2020–2021 ↗(On Diff #48122)

It might be not, but I want this change to be tested separately, if done. Also, at least for symmetry, other top level invalidation functions can get the same structure by the cost of one more if().

kib updated this revision to Diff 48323.Sep 21 2018, 5:35 PM

Reword comment, strip at eol.

This revision now requires review to proceed.Sep 21 2018, 5:35 PM
Harbormaster completed remote builds in B19733: Diff 48323.Sep 21 2018, 5:35 PM
alc accepted this revision.Sep 21 2018, 5:42 PM
This revision is now accepted and ready to land.Sep 21 2018, 5:42 PM
alc added inline comments.Sep 21 2018, 5:45 PM
sys/amd64/amd64/pmap.c
1801–1802 ↗(On Diff #48122)

Given recent changes, we could start using the kernel pmap cpu mask to track which processors actually require shootdown. I know that some of the sleep states actually flush the TLB based on experimental observation, but I don't know what is documented in this regard.

Closed by commit rS338862: Convert x86 TLB top-level invalidation functions to ifuncs. (authored by kib). · Explain WhySep 21 2018, 5:53 PM
This revision was automatically updated to reflect the committed changes.
kib marked an inline comment as done.Sep 21 2018, 6:01 PM
kib added inline comments.
sys/amd64/amd64/pmap.c
1801–1802 ↗(On Diff #48122)

You mean vmspace0.pmap, or kernel pmap ?

Anyway, documentation states that C3 writes back and invalidates CPU caches, but I do not remember any mention of the TLB flush.

kib mentioned this in D17277: Further reorganize pmap_invalidate TLB code..Sep 21 2018, 8:48 PM
alc added a comment.Sep 21 2018, 10:08 PM

kernel pmap. Specifically, I would propose clearing the processor's bit in the kernel pmap when it halts. That said, we would still need to have a separate flag indicating when the processor needs to take action when it wakes. (Depending on the documentation of the deeper sleep states maybe that action is a NOP.)

Revision Contents
Changeset List

PathSize
head/
sys/
amd64/
amd64/
365 lines

Diff 48325

View Options

head/sys/amd64/amd64/pmap.c

Loading...