Skip to content

cpu/fe310: Use ecall instruction for thread yield#15736

Merged
maribu merged 2 commits intoRIOT-OS:masterfrom
bergzand:pr/riscv/use_ecall
Jan 13, 2021
Merged

cpu/fe310: Use ecall instruction for thread yield#15736
maribu merged 2 commits intoRIOT-OS:masterfrom
bergzand:pr/riscv/use_ecall

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Jan 11, 2021

Contribution description

This switches the thread yield on the fe310 RISC-V CPU to use the ECALL instruction to trap the core. Currently the software interrupt is used for this, but this is supposed to be used for inter-core interrupts according to the manual.

I've kept the option open on the interface to add a number and some context to the ecall and have this passed to the trap handler. This still has to be hooked up to the trap handler itself in the future.

Testing procedure

This is only tested on Qemu as I currently don't have hardware to test this.

Thread switching on an fe310-based board, e.g. tests/bench_mutex_pingpong

I'm also curious about actual numbers, this improves performance for me on Qemu, but no clue if that's also the case on real hardware.

Issues/PRs references

Alternative to #15277

@bergzand bergzand added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Jan 11, 2021
@bergzand bergzand mentioned this pull request Jan 11, 2021
Closed
benpicco
benpicco reviewed Jan 11, 2021
View reviewed changes
maribu
maribu reviewed Jan 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Some comments inline, mostly style nits

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 11, 2021

@aabadie: Care to give this a spin on real hardware, to see the performance impact?

bergzand
bergzand commented Jan 11, 2021
View reviewed changes
@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jan 11, 2021
@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 11, 2021

@nmeum: Can you confirm that this works as expected with the simulator of yours?

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 11, 2021

Squash and Murdock?

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Jan 11, 2021

Squash and Murdock?

Squashed!

@bergzand bergzand added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 11, 2021
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jan 11, 2021

Care to give this a spin on real hardware, to see the performance impact?

Sure!

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jan 11, 2021

  • tests/bench_thread_yield_pingpong: { "result" : 683711, "ticks" : 468 }
  • tests/bench_thread_flags_pingpong: { "result" : 582833, "ticks" : 549 }

So that's even faster than master (compared to results given in #15277 (comment)).

aabadie
aabadie reviewed Jan 11, 2021
View reviewed changes
cpu/fe310/irq_arch.c
break;
}
default:
#ifdef DEVELHELP
Copy link
Copy Markdown
Contributor

@aabadie aabadie Jan 11, 2021

Choose a reason for hiding this comment

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

Would this work with if (IS_ACTIVE(DEVELHELP)) { ?

Copy link
Copy Markdown
Member Author

@bergzand bergzand Jan 11, 2021

Choose a reason for hiding this comment

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

Probably, but out of scope here. I'll add that to the list of cleanups for this file

@bergzand bergzand added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 11, 2021
@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 11, 2021

Shouldn't we also drop the handling of IRQ_M_SOFT in line 85? Feel free to squash right away.

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Jan 11, 2021

Shouldn't we also drop the handling of IRQ_M_SOFT in line 85? Feel free to squash right away.

Pushed a separate commit to remove it

@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 11, 2021

Let's wait for @nmeum to see if this also fixes his use case. But generally speaking, I like using ECALL due to the additional flexibility the arguments provide. Combined with a memory protection unit this could be used to implement a real syscall interface and a somewhat isolated user space. (We might for simplicity just share the heap for malloc() and friends between threads.)

And also: It is faster :-)

@nmeum
Copy link
Copy Markdown
Member

nmeum commented Jan 12, 2021

@nmeum: Can you confirm that this works as expected with the simulator of yours?

If it works with QEMU it should, currently a bit busy with other things and can't test it though but I don't see any reason why it shouldn't work ;)

maribu
maribu approved these changes Jan 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. This only affects two boards, which currently are not super wide-spread. Hence, I'd argue this PR has only a minor impact and should be good to go during soft feature freeze.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 12, 2021
@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 12, 2021

@jia200x: Last chance to block this PR during feature freeze ;-)

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 12, 2021

ACK. This only affects two boards, which currently are not super wide-spread. Hence, I'd argue this PR has only a minor impact and should be good to go during soft feature freeze.

Sure! I'm OK with it

@maribu maribu changed the title fe310: Use ecall instruction for thread yield cpu/fe310: Use ecall instruction for thread yield Jan 12, 2021
@maribu
Copy link
Copy Markdown
Member

maribu commented Jan 12, 2021

@bergzand: The commit messages are should IMO be prefixed cpu/fe310: rather than fe310:. Feel free to hit merge after force-pushing

@maribu maribu merged commit 605220c into RIOT-OS:master Jan 13, 2021
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Jan 13, 2021

Thanks for the review and benchmarks!

@bergzand bergzand deleted the pr/riscv/use_ecall branch January 13, 2021 13:04
@bergzand bergzand mentioned this pull request Jan 22, 2021
Merged
This was referenced Feb 5, 2021
Merged
Open
@jia200x jia200x mentioned this pull request Feb 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco left review comments

@aabadie aabadie aabadie left review comments

@maribu maribu maribu approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

Assignees

No one assigned

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@bergzand @maribu @aabadie @nmeum @jia200x @benpicco