runtime: make channels work in interrupts #1142

niaow · 2020-05-30T03:49:00Z

This works right now, but there are some pieces here and there which are sub-optimal or have limited support. This is mostly a proof of concept that I hacked together tonight.

src/runtime/chan.go

src/runtime/runtime_atsamd21.go

niaow · 2020-05-30T14:04:58Z

@aykevl Is this design more of what you were looking for, rather than the other scheduler design?

go.sum

aykevl

I still need to look at this properly (and I intend to do so very soon), but I quickly skimmed through the code.

go.sum

src/runtime/runtime_atsamd21.go

src/runtime/arch_cortexm.go

src/runtime/arch_tinygoriscv.go

src/runtime/chan.go

src/runtime/critical_cortexm.go

niaow · 2020-06-05T01:23:37Z

I don't want to scope-creep this PR any further, so I would suggest adding support for other boards in separate PRs after this is merged, because otherwise we would basically have to test every board to merge this.

deadprogram · 2020-06-09T06:05:50Z

@jaddr2line please resolve merge conflict.

niaow · 2020-06-09T13:24:23Z

Rebased.

niaow · 2020-06-13T13:52:27Z

Rebased again after the go mod tidy.

aykevl

I have taken a bit more in-depth look, I need to look at the write barrier next.

Sorry, lots of things going on right now in my life so don't have as much time as I would hope.

Sidenote (semi-related): the functionality I'm most waiting for is some sort of simple condition variable that one goroutine can wait on and an interrupt can signal. That's what is needed by the vast majority of hardware, for example to queue a DMA SPI transfer and wait for it to finish. Or, in my case, wait for a BLE operation to complete (in the nrf SoftDevice) while another goroutine is allowed to run. Right now I just wait for it to complete, which is a waste:

// Wait for the next advertisement packet to arrive.
// TODO: use some sort of condition variable once the scheduler supports
// them.
arm.Asm("wfe")
if gotScanReport.Get() == 0 {
	// Spurious event. Continue waiting.
	continue
}
gotScanReport.Set(0)

Of course, that should happen after this PR is merged (which lays the foundation for more interrupt <-> goroutine communication).

src/runtime/runtime_atsamd21.go

src/runtime/critical_cortexm.go

aykevl · 2020-06-14T01:59:26Z

I think the write barrier system is solid. I am however not sure whether disabling interrupts in the GC is ever a good idea. Even the possibility of it happening may be a problem for real time applications, while endless GC loops seem rather unlikely to me (except for really badly designed interrupt handlers). I'm inclined to say it's better to risk unbounded GC than to disable interrupts in the GC.

I'll think a bit about the best strategy.

aykevl · 2020-06-14T02:15:37Z

Actually, it seems to me that every heap pointer move would be a task pointer move from some arbitrary location (for example the blocked list in a channel) to the runqueue. Therefore it might be enough to scan the runqueue with interrupts disabled, or rather (after the heap has been scanned) do something like this:

while True:
  task = nil
  disable interrupts
    for t in runqueue:
      if t not marked:
        task = t
        break
  enable interrupts
  if task == nil:
    break # finished scanning
  scan task

Maybe we have already discussed this, but I don't see an obvious flaw here. The advantage here is that the number of goroutines is limited, especially on bare metal systems it may be just a few or only one. Certainly iterating through the runqueue will be much more predictable than rescanning the entire heap.

niaow · 2020-06-14T15:38:08Z

@aykevl I have removed the write barrier with a pattern like what you suggested.

niaow · 2020-06-14T19:47:38Z

Alright, this should finally be done!

deadprogram · 2020-06-15T16:52:05Z

I am doing some testing with this PR now...

deadprogram · 2020-06-15T18:23:50Z

So this PR currently causes the ItsyBitsy-M4 SAMD51 board to become unresponsive. I could recover by using the manual bootloader and then flashing code from the previous version...

niaow · 2020-06-15T18:30:10Z

Unresposive when running what?

deadprogram · 2020-06-15T19:01:24Z

Once flashed, the MCU "locks up" and even the USB CDC code no longer works. The IRQ that is uses to detect the USB CDC is no longer called, so the MCU no longer goes into bootloader when switching to 1200 baud.

deadprogram · 2020-06-18T18:36:45Z

When I was finally able to get GDB to connect (had a bad solder joint on my connector) here is the stack trace I was able to obtain:

(gdb) continue                                                                                                                                                               
Continuing.                                                                                                                                                                  
^C                                                                                                                                                                           
Program received signal SIGTRAP, Trace/breakpoint trap.                                                                                                                      
runtime.timerSleep (ticks=<optimized out>) at /home/ron/.gvm/pkgsets/go1.14.2/global/src/github.com/tinygo-org/tinygo/src/runtime/runtime_atsamd51.go:290                    
290             for timerWakeup.Get() == 0 {                                                                                                                                 
(gdb) bt                                                                                                                                                                     
#0  runtime.timerSleep (ticks=<optimized out>) at /home/ron/.gvm/pkgsets/go1.14.2/global/src/github.com/tinygo-org/tinygo/src/runtime/runtime_atsamd51.go:290                
#1  0x0000655c in runtime.sleepTicks (d=16384) at /home/ron/.gvm/pkgsets/go1.14.2/global/src/github.com/tinygo-org/tinygo/src/runtime/runtime_atsamd51.go:252                
#2  0x00006530 in runtime.scheduler () at /home/ron/.gvm/pkgsets/go1.14.2/global/src/github.com/tinygo-org/tinygo/src/runtime/scheduler.go:153                               
#3  0x00006456 in runtime.run () at /home/ron/.gvm/pkgsets/go1.14.2/global/src/github.com/tinygo-org/tinygo/src/runtime/scheduler_any.go:24                                  
#4  0x00006254 in Reset_Handler () at /home/ron/.gvm/pkgsets/go1.14.2/global/src/github.com/tinygo-org/tinygo/src/runtime/runtime_atsamd51.go:20

I discovered that the same problem of MCU locks up even on blinky1 exists on the nrf52840 when I accidentally flashed my Circuit Playground Bluefruit with code from this branch. Oops!

deadprogram · 2020-06-21T22:21:28Z

The issue I reported regarding M4 processors was corrected by #1182 which has now been merged into dev. @jaddr2line if you rebase dev into this branch then all the tests should pass now.

niaow · 2020-06-21T22:44:21Z

Rebased.

aykevl · 2020-06-23T22:15:47Z

I did a size comparison before and after this PR:

Somehow amd64 is affected by this: code size is usually increased a bit. I did not expect that to happen. Not a big problem but it might be a symptom of something.
Similarly, WebAssembly outputs tend to be a bit larger.
As expected, code size is increased a little bit for most baremetal targets (usually around 150-200 bytes). I think this increase is worth the extra functionality it provides.

niaow · 2020-06-23T22:25:46Z

Somehow amd64 is affected by this: code size is usually increased a bit.

This probably has to do with the GC changes.

aykevl · 2020-06-23T22:30:36Z

Right, that makes sense. For amd64 I don't really care but for wasm it's a bit unfortunate that it adds extra overhead for something that doesn't happen on WebAssembly.

niaow · 2020-06-23T23:04:45Z

I have now put parts of the conservative collector behind the baremetal build tag, and they will only build if the scheduler is enabled

The test failure is completely unrelated, and is an instance of #1185

deadprogram · 2020-06-24T06:42:34Z

@jaddr2line if you rebase against dev the tests should pass now due to 1ad6953

niaow · 2020-06-24T12:52:33Z

Rebased.

…etal with a scheduler

aykevl

I did another size comparison. The last commit (to reduce size) did not have an effect on amd64 but did reduce code size on wasm. Still there is a small (much more acceptable) size increase on wasm. Interestingly, it did sometimes reduce .bss size by 4 bytes on baremetal targets.

Size results (for diffing):
https://gist.github.com/aykevl/ea54c616224e5f8af55545cef7387d77

This is ready to go from my POV. @deadprogram any last comments before merging?

(some thoughts below inline, probably not worth changing)

aykevl · 2020-07-03T12:28:17Z

src/internal/task/queue.go


 // Push a task onto the queue.
 func (q *Queue) Push(t *Task) {
+	i := interrupt.Disable()


It might be more efficient to put all these locks where the queue/stack are used.

aykevl · 2020-07-03T12:30:28Z

src/runtime/gc_conservative.go

+	if baremetal && hasScheduler {
+		// Channel operations in interrupts may move task pointers around while we are marking.
+		// Therefore we need to scan the runqueue seperately.
+		var markedTaskQueue task.Queue


I was hoping you could just go through the runqueue instead of (essentially) rewriting it, but it seems fine. And perhaps safer.

deadprogram · 2020-07-04T06:33:56Z

From my POV this PR is ready to go. The small comments made by @aykevl are either not worth changing, or can be looked at for future refinements.

Now merging, incredible work again from @jaddr2line thank you!! 🍰 🎆

niaow commented May 30, 2020

View reviewed changes

src/runtime/chan.go Show resolved Hide resolved

niaow force-pushed the altsched branch 2 times, most recently from 82294f6 to da83522 Compare May 30, 2020 04:09

niaow commented May 30, 2020

View reviewed changes

src/runtime/runtime_atsamd21.go Show resolved Hide resolved

niaow mentioned this pull request May 30, 2020

runtime: chansend and chanrecv operations should not allocate #1143

Closed

niaow commented May 30, 2020

View reviewed changes

go.sum Show resolved Hide resolved

aykevl reviewed Jun 4, 2020

View reviewed changes

niaow force-pushed the altsched branch from c038a5b to 1dc963b Compare June 5, 2020 00:49

niaow force-pushed the altsched branch from 1dc963b to f128a4e Compare June 5, 2020 00:55

niaow marked this pull request as ready for review June 5, 2020 01:18

niaow requested a review from aykevl June 5, 2020 01:23

niaow added this to the v0.14 milestone Jun 6, 2020

niaow force-pushed the altsched branch from 7841bbe to ee59462 Compare June 9, 2020 13:23

niaow force-pushed the altsched branch from ee59462 to 76ef1a9 Compare June 13, 2020 13:51

aykevl reviewed Jun 13, 2020

View reviewed changes

src/runtime/runtime_atsamd21.go Outdated Show resolved Hide resolved

src/runtime/runtime_atsamd21.go Show resolved Hide resolved

src/runtime/critical_cortexm.go Outdated Show resolved Hide resolved

niaow force-pushed the altsched branch from 4a59515 to b44c02b Compare June 14, 2020 19:47

niaow mentioned this pull request Jun 15, 2020

runtime: add cheap atomic condition variable #1172

Merged

niaow force-pushed the altsched branch from b44c02b to 6d1f09e Compare June 21, 2020 22:44

niaow force-pushed the altsched branch from 955c02f to 02244d0 Compare June 24, 2020 12:52

niaow force-pushed the altsched branch from 02244d0 to 3e97b06 Compare June 27, 2020 17:39

niaow added 2 commits June 27, 2020 13:39

runtime: make channels work in interrupts

f15026d

runtime (gc): do not scan the runqueue when the platform is not barem…

3e97b06

…etal with a scheduler

niaow mentioned this pull request Jun 27, 2020

runtime (atsamd51): allow interrupts to wake the scheduler #1197

Merged

niaow modified the milestones: v0.14, v0.15 Jun 27, 2020

aykevl reviewed Jul 3, 2020

View reviewed changes

deadprogram merged commit e8c84d2 into tinygo-org:dev Jul 4, 2020

deadprogram mentioned this pull request Jul 4, 2020

proposal: "runtime/sync" package #712

Closed

niaow modified the milestones: v0.15, v0.14 Jul 4, 2020

aykevl mentioned this pull request Jul 4, 2020

riscv: add new kendryte k210 chip #1149

Merged

7 tasks

niaow mentioned this pull request Jul 6, 2020

runtime: allow interrupts to wake the scheduler #1214

Open

10 tasks

aykevl mentioned this pull request Feb 1, 2021

bluepill: fix sleep logic #1608

Closed

runtime: make channels work in interrupts #1142

runtime: make channels work in interrupts #1142

Uh oh!

Conversation

niaow commented May 30, 2020

Uh oh!

Uh oh!

Uh oh!

niaow commented May 30, 2020

Uh oh!

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niaow commented Jun 5, 2020

Uh oh!

deadprogram commented Jun 9, 2020

Uh oh!

niaow commented Jun 9, 2020

Uh oh!

niaow commented Jun 13, 2020

Uh oh!

aykevl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aykevl commented Jun 14, 2020

Uh oh!

aykevl commented Jun 14, 2020

Uh oh!

niaow commented Jun 14, 2020

Uh oh!

niaow commented Jun 14, 2020

Uh oh!

deadprogram commented Jun 15, 2020

Uh oh!

deadprogram commented Jun 15, 2020

Uh oh!

niaow commented Jun 15, 2020

Uh oh!

deadprogram commented Jun 15, 2020

Uh oh!

deadprogram commented Jun 18, 2020

Uh oh!

deadprogram commented Jun 21, 2020

Uh oh!

niaow commented Jun 21, 2020

Uh oh!

aykevl commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niaow commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aykevl commented Jun 23, 2020

Uh oh!

niaow commented Jun 23, 2020

Uh oh!

deadprogram commented Jun 24, 2020

Uh oh!

niaow commented Jun 24, 2020

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

aykevl Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

aykevl Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

aykevl left a comment •

edited

Loading

aykevl commented Jun 23, 2020 •

edited

Loading

niaow commented Jun 23, 2020 •

edited

Loading