Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal/task: use a lock-free queue #2290

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

niaow
Copy link
Member

@niaow niaow commented Nov 19, 2021

This changes task.Stack and task.Queue to use atomic compare-and-swap operations.
With this change, Push can also be executed safely from multiple threads/cores.
Additionally, the queue is actually LIFO now (not sure what I was thinking before).

Not entirely sure about this yet.

@niaow
Copy link
Member Author

niaow commented Nov 19, 2021

Oops, i meant FIFO not LIFO. Also xtensa seems to still need work?

@niaow
Copy link
Member Author

niaow commented Nov 19, 2021

Size impact statistics:

  • Arduino Uno Blinky (tasks): adds 172 bytes
  • Arduino Nano 33 Blinky: adds 84 bytes
  • Metro M4 Airlift Blinky: adds 40 bytes
  • ESP32-Mini32 Blinky: adds 40 bytes
  • Linux Channel Test: adds 76 bytes

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: do you have plans to use the changes in this PR? Any new features this will add?

I haven't looked too deeply into the queue code but I don't see anything surprising with a quick glance.

src/runtime/runtime_esp8266.go Outdated Show resolved Hide resolved
@@ -14,3 +17,14 @@ func align(ptr uintptr) uintptr {
}

func getCurrentStackPointer() uintptr

//export __sync_val_compare_and_swap_2
func __sync_val_compare_and_swap_2(ptr *uint32, expected, desired uint32) uint32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same goes for AVR.

Note that atomics are somewhat broken on AVR. If you see weird behavior on AVR, it could be the backend that is emitting invalid instructions.
For more details, see: https://reviews.llvm.org/D97127

src/runtime/runtime_avr.go Outdated Show resolved Hide resolved
@niaow
Copy link
Member Author

niaow commented Nov 20, 2021

Out of curiosity: do you have plans to use the changes in this PR?

I am not intending to do anything specific right now, but this might be useful once we want to implement multicore support for something.

Any new features this will add?

Right now:

  1. The queue. . . is actually a queue instead of a weird mostly-stack
  2. On devices with support for atomics, this does not need any critical sections

The Push operation can also be safely called concurrently. . . but only if the tasks are pinned safely (which is currently an issue, but this is more of a future thing).

@niaow
Copy link
Member Author

niaow commented Dec 28, 2021

This has been rebased now that #2307 is merged

@niaow
Copy link
Member Author

niaow commented Dec 28, 2021

I still need to test this fully on AVR though

This changes task.Stack and task.Queue to use atomic compare-and-swap operations.
With this change, they can also be executed safely from multiple threads/cores.
Additionally, the queue is actually LIFO now (not sure what I was thinking before).
@niaow niaow marked this pull request as ready for review January 15, 2022 15:01
@niaow
Copy link
Member Author

niaow commented Jan 15, 2022

Now that #2458 and #2459 are in, I consider this ready.

@deadprogram
Copy link
Member

Seems like this is perhaps not working correctly on the HiFive1b? I tried several times, and also ran some other test code successfully.

@jclab-joseph jclab-joseph mentioned this pull request Dec 18, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants