Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.44%. Comparing base (451ba1c) to head (5d8878b).
Report is 2 commits behind head on master.

@@           Coverage Diff           @@
##           master   #15448   +/-   ##
=======================================
  Coverage   98.43%   98.44%           
=======================================
  Files         163      163           
  Lines       21294    21296    +2     
=======================================
+ Hits        20960    20964    +4     
+ Misses        334      332    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:   +16 +0.002% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:   +40 +0.004% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Looks like there was a long standing underlying bug here when using rp2 threads, where either CPU may poll CDC input and trigger the TinyUSB task. TinyUSB could run on both CPUs concurrently, which may have lead to some incorrect behaviour.

I guess that was introduced in bcbdee2 ? Prior to that commit, tinyusb was guarded so it ran only on core 0.

I guess that was introduced in bcbdee2 ? Prior to that commit, tinyusb was guarded so it ran only on core 0.

I think that commit set up the conditions for the bug. TinyUSB still only runs on CPU0 as of this commit because that's where the scheduler runs it. However, that commit introduces a different bug - waiting for data in places where the scheduler doesn't run will hang. Commit a00c9d5 fixed that by re-introducing a TinyUSB task hook, but without the CPU check.

TinyUSB still only runs on CPU0 as of this commit because that's where the scheduler runs it.

Oh wait, I have this wrong don't I? mp_handle_pending() can run on either CPU... I didn't realise this...

(The bug still doesn't happen in that commit because the scheduler has a lock, but I think that this fix is incomplete as it doesn't handle the case where mp_usb_task is scheduled to run, the scheduler runs on CPU1, and simultaneously is polled from code running on CPU0...)

mp_handle_pending() can run on either CPU

Correct. Eg called from the VM itself.

The bug still doesn't happen in that commit because the scheduler has a lock, but I think that this fix is incomplete as it doesn't handle the case where mp_usb_task is scheduled to run, the scheduler runs on CPU1, and simultaneously is polled from code running on CPU0...)

Are you saying this PR is not correct ("this fix is incomplete"), or are you referring to the commit a00c9d5 not being fully correct?

Are you saying this PR is not correct ("this fix is incomplete")

This PR isn't a complete fix. I think there's still a race where:

• CPU0 is calling poll_cdc_interfaces() which runs mp_usbd_task (allowed by the CPU0 check)
• CPU1 runs the scheduler and this runs mp_usbd_task

The simple fix is to always guard the scheduled mp_usb_task to not run on CPU1, but because mp_usbd_task is run on-demand then there needs to be a way to re-schedule mp_usbd_task to run later, and keep retrying until it runs on CPU0. I have a fix for this here now, but I'm not sure about it (also requires a scheduler change).

Or, add proper locking around mp_usbd_task so it can run on either CPU.

I've pushed two commits which are a possible fix for this, but I don't like it much.

I think the smart fix here might be to revert bcbdee2 and re-introduce the dedicated TinyUSB task hook for CPU0, instead of calling TinyUSB via the scheduler. If the performance hit of that change is high then can keep the DCD event handler function and use it to set a flag that we check before calling into TinyUSB.

We need to think about this in depth, because all other bare-metal ports that use TinyUSB need to do the same thing.

We need to think about this in depth, because all other bare-metal ports that use TinyUSB need to do the same thing.

Agree about thinking it through. Currently we have rp2 and samd using the scheduler approach, and all of the other ports using a task hook.

The queue-via-scheduler approach should work fine on all the single core bare metal ports. It's only made complex in the dual core case where it's possible for both CPUs to be trying to call into the TinyUSB task at once.

Will think about it some more. Maybe adding another recursive mutex on rp2 is the way to go.

@projectgus and @jimmo and I discussed this and decided to fix it by only allowing scheduled functions to run on the main thread. See related #8838 and pybricks@b196c78

decided to fix it by only allowing scheduled functions to run on the main thread

I've implemented this "scheduler only on the main thread" change and it's working but I don't think it's a good solution after all. See this branch:
https://github.com/projectgus/micropython/commits/bugfix/rp2_tinyusb_recursive_mainthread/

It's a problem for ports with GIL and multiple threads (i.e. ESP32). The stress_schedule.py test really falls over, and I think real workloads will also suffer:

• vm.c:1358 only bounces the GIL if the scheduler is idle, not pending, which is a problem if only main thread can run the scheduler and another thread is running. Changing this to also bounce the GIL if pending on a non-main thread (similar to the pybricks patch) helps but it's still quite slow because the GIL only bounces every MICROPY_PY_THREAD_GIL_VM_DIVISOR branches. Therefore scheduled function latency can be quite high if another thread is running.
• Replaced that with a change in the linked branch, where scheduler.c bounces the GIL instead of running the scheduler on a non-main threads. This improves things a lot, but it's still higher latency because now the main thread has to take the GIL, then execute another iteration of the main VM loop, and only then it handles the scheduled operation. Whereas before the non-main thread would have run the scheduler immediately.

Going to take another look at this next week. @jimmo mentioned the idea of having a flag so some scheduler nodes only run on the main thread, this might be the best compromise. Otherwise maybe implementing a mutex for TinyUSB (with some caution around ensuring race conditions can't lose TinyUSB events despite the mutex).

I've implemented this "scheduler only on the main thread" change and it's working but I don't think it's a good solution after all.

Hmmm, OK, I'll also have a think about this. We do need to try and reduce the latency of scheduled callbacks, because they are being used more and more for background workloads.

Finally got a version of this fix I'm happy with, after @dpgeorge pointed out that this problem only exists where GIL is disabled.

(An aside, I also noticed that there is no user-facing documentation for the GIL at all. I made a note to submit something.)

@dlech This may be of interest, although I'm unsure if its equivalent to pybricks@b196c78 or not (specifically, I don't know if pybricks enables the GIL or not).

Yeah, we are using that patch on the Unix port with the GIL enabled. So it looks like this isn't quite the same. The problems were were trying to solve are A) Unix signal handlers should always run on the main thread, like in CPython and B) if there is an unhandled exception in any background task, we want it to stop the whole program by throwing in the main thread rather than throwing in a random thread.

@dlech That makes sense, thanks for the extra detail.

OK, so PyBrick's use case is more about functionality, rather than fixing a bug. And because it's on the unix port there's less concern about loss of performance / increase latency of scheduled code.

I have tested this latest PR on rp2 and it fixes the original report in #15390.

This turned out to be a relatively small patch, which is great. Thank you!