cpu/cortexm: "free" SVC by kaspar030 · Pull Request #13825 · RIOT-OS/RIOT

kaspar030 · 2020-04-06T19:17:00Z

Contribution description

Currently, the Cortex-M "supervisor call exception" SVC (think system call entry point) is completely blocked by being defined to be tail-called by PendSV, in order to call sched_run() and restore the context from sched_active_thread. This makes it unusable for any other use but cpu_switch_context_exit().

This PR refactors PendSV to include the previous SVC routine (run scheduler, restore thread context), but before that, only conditionally stores the current context only if sched_active_thread is non-null.

An SVC dispatch function is added. SVC 1 maps to the original function by handing over to PendSV.

This is the first step to implement system calls using SVC, which can now be used in combination with the MPU to provide some memory protected "user space" that still interacts with the kernel.

Performance wise, context switches are slightly faster on Cortex-M3 onwards, and slightly slower on CM0(+), but both times less than 0.5%. Crude measuring shows ~50b increase in code size.

I've tested this only on particle-xenon (Cortex-M4F) and nucleo-f072rb (Cortex-M0+).

Testing procedure

Being quite central with a lot of opportunities for weird error cases, this should get somewhat extensive review and testing, especially anything involving context switching on different Cortex-M.

Issues/PRs references

kaspar030 · 2020-04-28T07:56:26Z

I've pushed a commit that makes the dispatching optional on selecting the module cortexm_svc, basically restoring the previous default behavior.

fjmolinas

Code looks good, some nitpicks below. Will start testing.

cpu/cortexm_common/thread_arch.c

makefiles/pseudomodules.inc.mk

fjmolinas · 2020-04-28T08:11:16Z

cpu/cortexm_common/thread_arch.c

+    /* skip context saving if sched_active_thread == NULL */
+    "ldr    r1, =sched_active_thread  \n" /* r1 = &sched_active_thread  */
+    "ldr    r1, [r1]                  \n" /* r1 = sched_active_thread   */
+    "cmp    r1, #0                    \n" /* if r1 == NULL:             */
+    "beq context_restore              \n" /*     goto context_restore   */


Skipping context switching depending on sched_active_thread should go into its own commit.

ok, will re-shuffle when squashing

cpu/cortexm_common/thread_arch.c

fjmolinas · 2020-04-28T09:25:53Z

Bench-marking tests/bench_thread_yield_pingpong'

core	master	pr	pr + cortexm_svc
m0+ (usb-kw41z)	96632	95862	95862
m0 (nucle0-f091rc)	85683	85076	85076
m3 (openmote-b)	70123	70741	70741
m3 (iotlab-m3)	161035	158550	158550
m4 (frdm-k64f)	150376	149254	149254
m7 (nucleo-f767zi)	291104	289156	289156

In general they are all running faster. Only openmote-b had a .8% increase.

fjmolinas · 2020-04-28T09:37:56Z

tests/thread_cooperation still working correctly for the above mentioned boards, any other testing suggestion? I could run a subset of tests on many boards?

cpu/cortexm_common/thread_arch.c

kaspar030 · 2020-04-28T10:15:38Z

In general they are all running faster. Only openmote-b had a .8% increase.

Aehm, the benchmark counts context switch per second, so lower is better ;) edit sorry for adding my own confusion, higher numbers are better.

I'm seeing a slight improvement on nucleo-f401re.

fjmolinas · 2020-04-28T10:24:26Z

Aehm, the benchmark counts context switch per second, so lower is better ;) edit sorry for adding my own confusion, higher numbers are better.

Well then iotlab-m3 is slower.

cpu/cortexm_common/thread_arch.c

kaspar030 · 2020-04-28T10:53:55Z

Hm, I'm seeing different behavior gcc vs llvm, and gcc versions in our toolchain vs. 9.3.0, on both nucleo-f401re and samr21-xpro. llvm is always slightly slower on master (both current llvm and the one in our container). gcc in our container is slightly slower with this pr. current gcc is slightly faster with this pr. :-/

fjmolinas · 2020-04-28T11:22:06Z

Hm, I'm seeing different behavior gcc vs llvm, and gcc versions in our toolchain vs. 9.3.0, on both nucleo-f401re and samr21-xpro. llvm is always slightly slower on master (both current llvm and the one in our container). gcc in our container is slightly slower with this pr. current gcc is slightly faster with this pr. :-/

When using llvm on iotlab-m3 I get more context switches on master 176432 vs 173876 on this PR.

kaspar030 · 2020-05-05T10:10:45Z

So, with llvm, PR is faster. With gcc, this PR is generally slower. It might get faster with PR on newer gcc releases.

As we don't have an actual use case ATM, I guess we have to wait. No need to slow down context switches for nothing.

cpu/cortexm_common/Makefile.features

kaspar030 · 2020-06-05T12:48:04Z

@leandrolanzieri @jia200x can you take a look if I added the feature to Kconfig correctly?

leandrolanzieri · 2020-06-05T13:25:28Z

@leandrolanzieri @jia200x can you take a look if I added the feature to Kconfig correctly?

Yes, LGTM!

kaspar030 · 2020-06-08T11:09:19Z

This is now a requirement for #14224. IMO that makes the slight performance drop for some compilers / MCUs acceptable...

maribu · 2020-06-08T12:16:05Z

IMO that makes the slight performance drop for some compilers / MCUs acceptable...

Actually, now is the perfect time to add some features coming at the price of reduced performance: The inline-able IRQ API improved the performance so much, that the next release is still going to be faster than the current one. So users won't notice 😈

fjmolinas

It seems there is an agreement on that this feature is useful, and that its sideffects have been mitigated by the inline-able irq api. <1% decrease here vs 22% in the inline irq api. Also running the same bench mark on #14224 vs this PR also yields a ~1% increase on context switches, so IMO there is nothing blocking this anymore.

The code hasn't changed since the last time I tested except for the kconfig FEATURE part. Since this touches core I would still like to get a second ACK here, maybe @maribu @bergzand?

fjmolinas · 2020-06-09T08:44:48Z

I would like to see a run of all tests here as well.

fjmolinas · 2020-06-09T12:36:26Z

Test failures are in esp32, so unrelated, I'll re-trigger without them

benpicco · 2020-06-10T13:31:40Z

Now a rebase is needed.

fjmolinas · 2020-06-16T08:31:03Z

No one has said anything for while so GO!

kaspar030 · 2020-06-16T08:31:43Z

No one has said anything for while so GO!

Thanks for reviewing!

kaspar030 requested review from aabadie, bergzand, dylad and fjmolinas April 6, 2020 19:17

kaspar030 requested review from DipSwitch, haukepetersen and thomaseichinger as code owners April 6, 2020 19:17

kaspar030 force-pushed the cortexm_free_svc branch from 22f2ce2 to eac0255 Compare April 28, 2020 07:53

fjmolinas self-assigned this Apr 28, 2020

fjmolinas reviewed Apr 28, 2020

View reviewed changes

cpu/cortexm_common/thread_arch.c Outdated Show resolved Hide resolved

fjmolinas reviewed Apr 28, 2020

View reviewed changes

cpu/cortexm_common/thread_arch.c Outdated Show resolved Hide resolved

fjmolinas reviewed Apr 28, 2020

View reviewed changes

cpu/cortexm_common/thread_arch.c Outdated Show resolved Hide resolved

fjmolinas reviewed Apr 28, 2020

View reviewed changes

cpu/cortexm_common/thread_arch.c Outdated Show resolved Hide resolved

fjmolinas mentioned this pull request May 11, 2020

cpu/cortexm_common: add inlined header only def for irq_% #13999

Merged

fjmolinas reviewed May 27, 2020

View reviewed changes

cpu/cortexm_common/Makefile.features Show resolved Hide resolved

kaspar030 force-pushed the cortexm_free_svc branch from 726303a to d804d7a Compare June 4, 2020 19:10

kaspar030 requested a review from cgundogan as a code owner June 5, 2020 12:47

kaspar030 requested review from jia200x and leandrolanzieri as code owners June 5, 2020 12:47

kaspar030 mentioned this pull request Jun 8, 2020

core: make idle thread optional #14224

Merged

fjmolinas approved these changes Jun 9, 2020

View reviewed changes

fjmolinas added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before labels Jun 9, 2020

kaspar030 force-pushed the cortexm_free_svc branch from 2009977 to ab28db7 Compare June 9, 2020 08:46

kaspar030 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 Jun 9, 2020

kaspar030 added 3 commits June 10, 2020 23:12

cpu/cortexm: "free" SVC

dbe7331

makefiles/features_modules: add cortexm_svc

24facae

cpu/cortexm_common/Kconfig: add cortexm_svc feature

8466946

kaspar030 force-pushed the cortexm_free_svc branch from ab28db7 to 8466946 Compare June 10, 2020 21:13

fjmolinas merged commit b041221 into RIOT-OS:master Jun 16, 2020

kaspar030 deleted the cortexm_free_svc branch June 16, 2020 08:31

miri64 added this to the Release 2020.07 milestone Jun 24, 2020

Conversation

kaspar030 commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kaspar030 commented Apr 28, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjmolinas Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

kaspar030 Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Apr 28, 2020

Uh oh!

Uh oh!

kaspar030 commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Apr 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaspar030 commented Apr 28, 2020

Uh oh!

fjmolinas commented Apr 28, 2020

Uh oh!

kaspar030 commented May 5, 2020

Uh oh!

Uh oh!

kaspar030 commented Jun 5, 2020

Uh oh!

leandrolanzieri commented Jun 5, 2020

Uh oh!

kaspar030 commented Jun 8, 2020

Uh oh!

maribu commented Jun 8, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jun 9, 2020

Uh oh!

fjmolinas commented Jun 9, 2020

Uh oh!

benpicco commented Jun 10, 2020

Uh oh!

fjmolinas commented Jun 16, 2020

Uh oh!

kaspar030 commented Jun 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kaspar030 commented Apr 6, 2020 •

edited

Loading

fjmolinas commented Apr 28, 2020 •

edited

Loading

kaspar030 commented Apr 28, 2020 •

edited

Loading