py/scheduler: Fix race with scheduler and pending exceptions.

[jimmo]

Alternative to #8838 that keeps per-thread pending exceptions. cc @dlech

The optimisation that allows a single check in the VM for either a pending exception or non-empty scheduler queue doesn't work when threading is enabled, as one thread can clear the sched_state if it has no pending exception, meaning the thread with the pending exception will never see it.

This removes that optimisation for threaded builds, and also makes it so the scheduler only runs on the main thread (matching CPython behaviour). See #8838 (comment) for more background.

This retains the single-check optimisation for the non-threading build, so performance is unaffected (actually marginally better). I will check on other ports.

The code size diff for PYBV11 is -56 bytes.

$ ./run-perfbench.py -s ~/sched-split-baseline ~/sched-split-v4
diff of scores (higher is better)
N=100 M=100                /home/jimmo/sched-split-baseline -> /home/jimmo/sched-split-v4         diff      diff% (error%)
bm_chaos.py                    359.51 ->     362.73 :      +3.22 =  +0.896% (+/-0.01%)
bm_fannkuch.py                  77.39 ->      77.32 :      -0.07 =  -0.090% (+/-0.00%)
bm_fft.py                     2517.61 ->    2527.57 :      +9.96 =  +0.396% (+/-0.00%)
bm_float.py                   5720.17 ->    5801.24 :     +81.07 =  +1.417% (+/-0.00%)
bm_hexiom.py                    46.21 ->      46.35 :      +0.14 =  +0.303% (+/-0.00%)
bm_nqueens.py                 4394.03 ->    4427.44 :     +33.41 =  +0.760% (+/-0.00%)
bm_pidigits.py                 634.95 ->     634.96 :      +0.01 =  +0.002% (+/-0.35%)
misc_aes.py                    406.40 ->     411.67 :      +5.27 =  +1.297% (+/-0.01%)
misc_mandel.py                3478.51 ->    3487.97 :      +9.46 =  +0.272% (+/-0.00%)
misc_pystone.py               2424.89 ->    2426.23 :      +1.34 =  +0.055% (+/-0.01%)
misc_raytrace.py               371.76 ->     377.60 :      +5.84 =  +1.571% (+/-0.00%)

[jimmo]

I have updated this to only fix the race. Will leave the scheduler-on-main-thread for another PR.

[dlech]

The logic looks sound to me.

One thing that jumped out at me is that there is a potential difference of behavior with regard to pending exceptions when the scheduler is locked (this difference also existed before these changes so probably deserves a separate issue/PR if it is something to be concerned about). If the scheduler is disabled at compile time, then pending exceptions can be raised at times when the scheduler would be locked (in interrupt handlers, finalizers, etc). But if the scheduler is enabled then pending exceptions won't be raised during this time. With this PR, the condition also depends on threads being enabled or not but both behaviors still are possible.

[jimmo]

One thing that jumped out at me is that there is a potential difference of behavior with regard to pending exceptions when the scheduler is locked (this difference also existed before these changes so probably deserves a separate issue/PR if it is something to be concerned about). If the scheduler is disabled at compile time, then pending exceptions can be raised at times when the scheduler would be locked (in interrupt handlers, finalizers, etc). But if the scheduler is enabled then pending exceptions won't be raised during this time. With this PR, the condition also depends on threads being enabled or not but both behaviors still are possible.

That's a good point... We should definitely re-visit that as part of a discussion of moving the scheduler to the main thread only (I'd also like to consider per-thread scheduling... e.g. the ability to "schedule on same thread" etc).

[dpgeorge]

Do we need to be careful here to make an atomic read-then-clear of mp_pending_exception? At the moment it is:

mp_obj_t obj = MP_STATE_THREAD(mp_pending_exception);
MP_STATE_THREAD(mp_pending_exception) = MP_OBJ_NULL;

What happens if an IRQ runs between those two statements and changes mp_pending_exception? Eg if it's a ctrl-C (that overrides an existing non-ctrl-C pending exception) then that ctrl-C will be lost.

Also, the existing code checked that obj != MP_OBJ_NULL after obtaining the atomic lock. Shouldn't the new code also do this?

Same comments apply to the new mp_handle_pending() function.

[jimmo]

Updated. I don't think the original code handled this case when the scheduler was disabled, but it's now protected for both configurations. Also added back the double check for pending exception.

[jimmo]

Code size diff is now -24 bytes.

Thinking about this some more, I'm not entirely sure I understand what the benefit for inlining this in the VM is... surely the only thing that needs to be done in the VM is the fast path to check sched_state/pending_exc, the actual processing can be offloaded by a call to mp_handle_pending -- I will send another PR as a comparison. (Edit: see #8869)

[dpgeorge]

Closed in favour of #8869.