Skip to content

UEFI: add support for tasks scheduler and make it default - #5553

Open
sparques wants to merge 3 commits into
tinygo-org:devfrom
sparques:pr/uefi-tasks-scheduler
Open

UEFI: add support for tasks scheduler and make it default #5553
sparques wants to merge 3 commits into
tinygo-org:devfrom
sparques:pr/uefi-tasks-scheduler

Conversation

@sparques

Copy link
Copy Markdown
Contributor

This adds support to uefi-amd64 target for the tasks scheduler and makes it default. The none-scheduler is still supported.

I did a very basic test of making sure two separate goroutines ran simultaneously--seems to work.

I'm not sure if the bit in compileopts/config.go is kosher; Is that the right approach or is there a better way to conditionally include ExtraFiles?

@deadprogram

Copy link
Copy Markdown
Member

@sparques

Copy link
Copy Markdown
Contributor Author

I'm sorry, if there is something that's making src/internal/task/task_stack_avr.S conditionally included, I'm missing it. Does it not matter if an assembly file is included but not used?

@deadprogram

Copy link
Copy Markdown
Member

I am wonder why you would need it conditionally included when you can just add to target file?

@sparques

Copy link
Copy Markdown
Contributor Author

@deadprogram Ah, I think I see what was confusing me. I thought src/internal/task/task_stack_amd64_windows.S was specific to just the tasks scheduler (and thus, should not be included for any other scheduler). But it's fine if it's included even if we're not using the tasks scheduler?

@deadprogram

Copy link
Copy Markdown
Member

Here are some further edited comments from automated review.

  1. task_stack_amd64_winabi.go is a near-verbatim copy of task_stack_amd64_windows.go

Since both share the same .S file too, I'd delete the new file and update the tags instead:

  • task_stack_amd64_windows.go: //go:build scheduler.tasks && amd64 && (windows || uefi)
  • task_stack_amd64.go: //go:build scheduler.tasks && amd64 && !windows && !uefi (as in the PR)

That drops 58 lines and, more importantly, means a future fix to the register layout can't be applied to one copy and missed in the other. Renaming the file to task_stack_amd64_winabi.go at that point would be a nice touch, but it's cosmetic.

  1. ExtraFiles() in compileopts/config.go
  • Putting it in Config.ExtraFiles() rather than in target.go is actually the more correct of the two options, because c.Scheduler() reflects a -scheduler= command-line override, whereas the existing asyncify precedent (compileopts/target.go:229) keys off spec.Scheduler and would silently ignore the flag. So I'd keep it here rather than moving it.
  • The simpler alternative is just adding src/internal/task/task_stack_amd64_windows.S to extra-files in targets/uefi-amd64.json unconditionally, next to src/runtime/asm_amd64_windows.S which is already there. With scheduler=none the .S would reference tinygo_task_exit, which doesn't exist. If it links, that's ~5 fewer lines of special-casing in shared code. If it doesn't, the config.go approach is justified and I'd say so in a comment there.

Minor: the c.GOARCH() == "amd64" guard is redundant today (uefi-amd64 is the only uefi target and pins goarch: amd64), but harmless as future-proofing.

  1. schedulerSleepCustom busy-waits instead of using the sleep queue

for ticks() < deadline {
gosched()
}

The cooperative scheduler's normal path (addSleepTask + task.Pause(), scheduler_cooperative.go:253) should already work on UEFI because ticks(), nanosecondsToTicks(), and sleepTicks() are all implemented in src/runtime/runtime_uefi.go. If the default path works, this file can go away entirely.

If it does need to stay, with this implementation a sleeping goroutine spins the run queue for the whole duration, so time.Sleep never lets the scheduler go idle. That matches existing UEFI behavior (sleepTicks already spins on CpuPause), so it's not a regression but it does mean waitForEvents/SetWaitForEvents never gets a chance to run during a sleep, which may matter for anyone using that hook. Worth a comment in the file either way explaining why the standard sleep queue isn't used.

Also: the //go:linkname gosched runtime.Gosched is unnecessary since sleep_custom_uefi.go is in package runtime, so it can call Gosched() directly. The linkname adds an indirection with no benefit.

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.

2 participants