Page MenuHomeFreeBSD

i386 PAE: avoid atomic for pte_store().
ClosedPublic

Authored by kib on Feb 17 2019, 7:48 PM.

Details

Summary

Instead carefully write upper word, and only than the lower word with PG_V.

It provides some measurable system time saving on buildworld:

XX 114.33r  578.88u 101.51s  0.38i  680.77t 233.87I 277X i386-12n-nopae         
XX 116.05r  583.42u 112.93s  0.57i  696.92t 231.46I 301X i386-12n-pae           
XX 115.54r  584.18u 109.33s  0.52i  694.03t 230.25I 302X i386-12n-pae-patched

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 22577

Event Timeline

kib retitled this revision from i386: avoid atomic for pte_store(). to i386 PAE: avoid atomic for pte_store()..

It looks like this is assuming that the old value is not valid, but that's not always true. For example, pmap_promote_pde() simply overwrites the old PDE.

I don't really see how pmap_kextract() is safe wrt torn writes.

It looks like this is assuming that the old value is not valid, but that's not always true. For example, pmap_promote_pde() simply overwrites the old PDE.

I don't really see how pmap_kextract() is safe wrt torn writes.

It could only work for user pmaps, so I can check this. The pmap lock must be owned there.
For promotions, we can safely write zero then write the update, I believe.

In fact half-written ptes open up L1TF CPU bug, but it is so transient that I do not think we should care.

In D19226#411357, @kib wrote:

It looks like this is assuming that the old value is not valid, but that's not always true. For example, pmap_promote_pde() simply overwrites the old PDE.

I don't really see how pmap_kextract() is safe wrt torn writes.

It could only work for user pmaps, so I can check this. The pmap lock must be owned there.
For promotions, we can safely write zero then write the update, I believe.

Perhaps add a second function, pte_store_quick() or so, that requires the previous entry to be invalid, and use that in pmap_enter()? I suspect that most of the performance improvement comes from overwrites of invalid PTEs, i.e., there is not much benefit in avoiding the locked cmpxchg in pmap_promote_pde().

Define pte_store_z() to be lockless pte_store() and only use it for zero ptes in usermode pmaps.

markj added inline comments.
sys/i386/include/pmap_pae.h
105

Is _z for _zero? I think it might as well be spelled out if so.

Perhaps assert that PG_V is not set in the old entry.

This revision is now accepted and ready to land.Feb 19 2019, 2:07 AM
sys/i386/i386/pmap.c
4107

Hmm, I think you can unconditionally use pte_store_z() here since the old entry is known to be invalid.

sys/i386/i386/pmap.c
4107

But then it returns the question of random pmap_kextract() seeing torn write.
(I think that it would not access the mappings created by pmap_enter_quick_locked, but this is on principle to not allow torn writes for kernel_pmap).

sys/i386/i386/pmap.c
4107

Don't you still have this problem in the pmap_enter() change then?

I suspect it is a bug to call pmap_kextract() on an unmapped address, and this reasoning can be used to justify the use of pte_store_z() on invalid entries, even in the kernel pmap. I believe the issue is only with stores that modify a valid PTE or PDE in the kernel pmap.

kib marked an inline comment as done.Feb 19 2019, 5:50 PM
kib added inline comments.
sys/i386/i386/pmap.c
4107

I would have the answer to this question if Peter can reproduce the torn write issue with the patch. It is not even clear to me if e.g. pmap_kenter() or pmap_qenter() can use pte_store_zero().

pte_store_zero. Unconditionally use it in pmap_enter_quick_locked.

This revision now requires review to proceed.Feb 19 2019, 5:51 PM
markj added inline comments.
sys/i386/i386/pmap.c
4107

Do either of those routines get used often on i386?

sys/i386/include/pmap_pae.h
104

I think style(9) puts do { on the same line as the #define.

This revision is now accepted and ready to land.Feb 19 2019, 7:08 PM
kib marked an inline comment as done.Feb 19 2019, 7:32 PM
kib added inline comments.
sys/i386/i386/pmap.c
4107

pmap_qenter() absolutely, it is used when buffer needs to be mapped, which is always for a buffer with metadata. pmap_kenter() not so much, AFAIR.

This revision was automatically updated to reflect the committed changes.