Page MenuHomeFreeBSD

amd64 pmap: block on turnstile for lock-less DI
ClosedPublic

Authored by kib on Jun 14 2019, 12:53 PM.

Details

Summary

Port the code to block on turnstile to lock-less delayed invalidation, instead of yielding. Yield might cause tight loop due to priority inversion.

Since it is impossible to avoid race between block and wake-up, arm 1-tick callout to wakeup when thread blocks itself.

Reported and tested by: mjg

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Jun 14 2019, 12:53 PM
alc accepted this revision.Jun 16 2019, 5:17 PM
This revision is now accepted and ready to land.Jun 16 2019, 5:17 PM
markj added inline comments.Jun 16 2019, 7:33 PM
sys/amd64/amd64/pmap.c
1000 ↗(On Diff #58624)

Why do you recheck the loop condition here?

alc added a comment.Jun 17 2019, 2:09 AM

I would suggest that you add a comment to the code explaining why the callout is necessary.

kib added a comment.Jun 17 2019, 8:10 AM
In D20636#446616, @alc wrote:

I would suggest that you add a comment to the code explaining why the callout is necessary.

I will do it when I will be back at my workstation (in about a week).

sys/amd64/amd64/pmap.c
1000 ↗(On Diff #58624)

I do it to claim that the only race is due to the lost wakeup. We have in the thread finishing DI:

store generation (atomic, works as seq consistent fence)
load pmap_invl_waiters

Without re-check, the order in the waiting thread is

load generation
increment pmap_invl_waiters (seq cst)

which is reverse order to what is needed to make the increment fence to work against the store generation fence. With the re-check, the order becomes

increment pmap_invl_waiters (seq cst)
load generation

You might argue that the callout would close this race anyway, but callout doing the real work means that the thread lost up to 1 tick of the wall time sitting blocked instead of making progress. So leaving callout use only to wakeup race proper is useful.

markj added inline comments.Jun 17 2019, 4:17 PM
sys/amd64/amd64/pmap.c
1000 ↗(On Diff #58624)

Thanks. I think this should be explained in a comment.

kib updated this revision to Diff 58906.Jun 23 2019, 9:18 AM

Add comments explaining the callout use and recheck.

This revision now requires review to proceed.Jun 23 2019, 9:18 AM
alc added inline comments.Jun 23 2019, 7:48 PM
sys/amd64/amd64/pmap.c
999 ↗(On Diff #58906)

should be "page's"

1000 ↗(On Diff #58906)

I would say, "... below the current thread's number. Prepare to block so that we do not waste CPU cycles or worse, suffer livelock."

1004 ↗(On Diff #58906)

"... impossible to block without ..."

1007 ↗(On Diff #58906)

"... incrementing ..."

1014 ↗(On Diff #58906)

"... thread's invalidation ..." (Note the spelling correction to "invlidation".)

kib marked 5 inline comments as done.Jun 23 2019, 7:57 PM
kib updated this revision to Diff 58921.

Language corrections.

alc added inline comments.Jun 23 2019, 8:31 PM
sys/amd64/amd64/pmap.c
1008 ↗(On Diff #58921)

Verb tense: "... which will unblock us if we lose ..."

1014 ↗(On Diff #58921)

"Re-check the current ..."

kib updated this revision to Diff 58926.Jun 23 2019, 8:39 PM

Two more language fixes.

alc accepted this revision.Jun 23 2019, 8:42 PM
This revision is now accepted and ready to land.Jun 23 2019, 8:42 PM
markj accepted this revision.Jun 23 2019, 8:46 PM
This revision was automatically updated to reflect the committed changes.