Page MenuHomeFreeBSD

powerpc: Avoid deadlock when CPUs exchange threads
AcceptedPublic

Authored by mchoo on Wed, Sep 16, 8:50 PM.
Tags
None
Referenced Files
F175115118: D59742.diff
Thu, Oct 8, 9:04 AM
Unknown Object (File)
Tue, Oct 6, 3:19 AM
Unknown Object (File)
Sun, Oct 4, 2:06 AM
Unknown Object (File)
Sat, Oct 3, 1:54 PM
Unknown Object (File)
Sat, Oct 3, 6:18 AM
Unknown Object (File)
Sat, Oct 3, 12:19 AM
Unknown Object (File)
Thu, Oct 1, 11:01 AM
Unknown Object (File)
Tue, Sep 29, 8:05 AM
Subscribers

Details

Reviewers
jhb
adrian
jhibbits
Group Reviewers
PowerPC
scheduler
Summary

The PowerPC context switch waited for the incoming thread to leave
blocked_lock before releasing the outgoing thread. If two CPUs selected
each other's running threads concurrently, both CPUs waited for the
other to perform the release and neither could progress. This appeared
as intermittent boot hangs under QEMU MTTCG when taskqueue threads were
bound to opposite CPUs.

8df2e5421468 ("powerpc: put the isync inside the TD_LOCK()...")
attempted to fix the same hang by moving isync into the polling loop.
That did not address the circular wait: isync cannot cause either CPU to
execute the release store after the loop. It only added context
synchronization to failed iterations and changed timing. Move isync back
to the successful exit, which preserves acquire ordering for subsequent
context loads.

Match the ordering used by other architectures: publish the outgoing
thread's new lock before waiting for the incoming thread. Apply the
correction to both 32-bit and 64-bit switch paths.

The 2015 change moved the release after the stack switch because the old
implementation continued through pmap_activate() and optional state
restoration on the outgoing thread's stack after making that thread
runnable. The switch path has since changed: after the release it only
polls wired td_lock state using registers, then immediately loads the
incoming PCB_SP before updating per-CPU state or making any calls.
cpu_switch() also runs with external and decrementer interrupts disabled
by spinlock_enter(). Thus this does not restore the post-release call
window that the 2015 change corrected.

Fixes: 53607fe3cc8d ("Fix an extremely subtle concurrency bug...")
Fixes: 7a49d964d3f9 ("Merge r278429 from ppc64:")
Fixes: 8df2e5421468 ("powerpc: put the isync inside the TD_LOCK()...")
MFC after: 2 weeks
MFC to: stable/14, stable/15
Sponsored by: FreeBSD Foundation

Test Plan

No more random hangs observed on QEMU multithreaded TCG (20 trials)

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
No Test Coverage
Build Status
Buildable 77421
Build 74304: arc lint + arc unit

Event Timeline

mchoo requested review of this revision.Wed, Sep 16, 8:50 PM

oooo! i found and thought i fixed one of these a while back. lemme try this locally to see if it also fixes my boot time hangs!

The main fix is certainly correct, but I'm not sure the description of the isync change is correct and I'd be tempted to leave it out of this fix.

sys/powerpc/powerpc/swtch32.S
149

I think the isync in the loop still had acquire semantics, you are just doing fewer isyncs, so the description in the commit log doesn't seem accurate to me:

Move isync
after the wait so subsequent context loads retain acquire ordering.

I wonder if the isync in this case being in the loop was acting a bit like the x86 PAUSE instruction?

sys/powerpc/powerpc/swtch32.S
149

isync is expensive and its execution should be minimized. When it's in the loop, isync instructions except for the last iteration (when it no longer branches back to blocked_loop) does nothing meaningful.

isync had been outside the branch until @adrian moved it inside the branch in 8df2e542146801fd01675e56724eaa567d04c209 a few months ago. I believe this was a false fix because 1) I could still observe the bug in qemu and 2) debugging through qemu's gdb server showed that this was a deadlock issue.

sys/powerpc/powerpc/swtch32.S
149

yup, if you've nailed it then yay! i'm glad that expensive isync is going back where it belongs.

This revision is now accepted and ready to land.Tue, Oct 6, 3:16 AM