Fix pmap_invalidate_all() for kernel_pmap on x86 CPUs with PG_G support.
ClosedPublic
Actions

Authored by kib on Dec 2 2015, 11:03 AM.

Details

Reviewers

alc
cem
jhb
pho

Commits

rS291688: For amd64 non-PCID machines, and for i386 machines with support for

Summary

As jhb noted, pmap_invalidate_all() on any x86 CPU without PCID support does not flush global TLB entries. This patch aims to fix the issue.

Namely, it adds invltlb_glob() for i386 (and renames amd64 invltl_globpcid() to _glob()). The invltlb_glob() is used for kernel_pmap when global shutdown is requested. Since we need to know the pmap in the shutdown handler on i386 (on amd64 it is already stored in a global var), the i386 and amd64 TLB shutdown code become identical and are merged.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kib updated this revision to Diff 10665.Dec 2 2015, 11:03 AM

kib retitled this revision from to Fix pmap_invalidate_all() for kernel_pmap on x86 CPUs with PG_G support..

kib updated this object.

kib edited the test plan for this revision. (Show Details)

kib added reviewers: alc, jhb, cem.

kib set the repository for this revision to rS FreeBSD src repository - subversion.

kib added a subscriber: pho.

Herald added subscribers: rpokala, imp. · View Herald TranscriptDec 2 2015, 11:03 AM

kib mentioned this in D4280: pmap_invalidate_range: For very large ranges, flush the whole TLB.Dec 2 2015, 11:04 AM

Thanks, merging these is a definite improvement.

sys/amd64/amd64/mp_machdep.c
408 ↗	(On Diff #10665)	Could this be in a header instead? We don't have a sys/x86/include/smp.h yet (might be nice to have one that at least defined prototypes, etc. for the bits in sys/x86/x86/mp_x86.c). You could just put this in sys/amd64/include/smp.h for now next to smp_tlb_wait.

This revision is now accepted and ready to land.Dec 2 2015, 11:14 AM

Move extern *smp_tlb_pmap to amd64/include/smp.h.

This revision now requires review to proceed.Dec 2 2015, 11:52 AM

jhb accepted this revision.Dec 2 2015, 12:16 PM

jhb edited edge metadata.

This revision is now accepted and ready to land.Dec 2 2015, 12:16 PM

cem accepted this revision.Dec 2 2015, 5:16 PM

cem edited edge metadata.

Tested on SMP i386 for 19 hours, on amd64 with vm.pmap.pcid_enabled=1 for 19 hours and on amd64 with vm.pmap.pcid_enabled=0 for 11 hours.
No problems seen.

Closed by commit rS291688: For amd64 non-PCID machines, and for i386 machines with support for (authored by kib). · Explain WhyDec 3 2015, 11:14 AM

This revision was automatically updated to reflect the committed changes.

alc added inline comments.Dec 3 2015, 6:31 PM

head/sys/i386/i386/pmap.c
658	This conversion doesn't make sense to me. The objective of this function is to add the PG_G bit to existing kernel PTEs, so flushing just the non-PG_G TLB entries made sense (even if it's arguably unnecessary). That said, elsewhere, we assume that either PG_G or PG_PS implies the existence of invlpg, so this could be invlpg(), rather then invltlb().
667	Same here.

kib mentioned this in D4368: Use invlpg instead of total invalidation, when setting PG_G..Dec 4 2015, 8:35 AM

kib added inline comments.

head/sys/i386/i386/pmap.c
658	The comment makes me think that the invalidation is done to avoid some processor issues (erratas ?). Invalidation is formally not needed there, I believe, since only PG_G bit is changed, and both old and new pte provide same translations otherwise. If no CPU errata is in action, then the nearest context switch would do the same. BTW, all x86 processors supported by FreeBSD have inlpg. The instruction was added in 486 generation. D4368