cpu/msp430_common: Update to inline-able IRQ API #14362

maribu · 2020-06-25T19:54:38Z

Contribution description

Updated to inline-able IRQ API
Improved robustness of functions
- Added memory barrier to prevent the compiler from moving code outside of
  a critical section guarded by irq_disable() ... irq_restore()
Reduced overhead of irq_disable()
- After clearing the global interrupt enable (GIE) bit, IRQs remain enabled
  for up to one CPU cycle
- The previous implementation just added a nop to fill that cycle
- This implementation uses the cycle for masking the return value
Reduced overhead of irq_restore()
- Now only one CPU cycle is needed
irq_disable(), irq_restore(), and irq_enable() work now in constant time

Testing procedure

`tests/irq_disable_restore`

I ran tests/irq_disable_restore on a wsn430-v1_3b board in the FIT/IoT-Lab:

Console output

s
START
main(): This is RIOT! (Version: 2020.07-devel-1557-g22d44-msp430-irq-inline)
Test for irq_disable() / irq_restore()
======================================

Verifying test works: [SUCCESS]
Test result: [SUCCESS]

`tests/tests/bench_mutex_pingpong`

I ran tests/tests/bench_mutex_pingpong on the same hardware in the FIT/IoT-Lab

Console output with `master`

r
READY
s
START
main(): This is RIOT! (Version: 2020.07-devel-1557-g22d44)
main starting
{ "result" : 9222 }

Console output with this PR

READY
s
START
main(): This is RIOT! (Version: 2020.07-devel-1558-gc5c83-msp430-irq-inline)
main starting
{ "result" : 9910 }

So ~7.5% speedup.

Issues/PRs references

Follow up of: #13999

Tracked in: #14356

- Updated to inline-able IRQ API - Improved robustness of functions - Added memory barrier to prevent the compiler from moving code outside of a critical section guarded by irq_disable() ... irq_restore() - Reduced overhead of `irq_disable()` - After clearing the global interrupt enable (GIE) bit, IRQs remain enabled for up to one CPU cycle - The previous implementation just added a nop to fill that cycle - This implementation uses the cycle for masking the return value - Reduced overhead of `irq_restore()` - Now only one CPU cycle is needed - `irq_disable()`, `irq_restore()`, and `irq_enable()` work now in constant time

fjmolinas · 2020-06-26T09:35:41Z

I'll test on z1

aabadie · 2020-06-26T09:52:57Z

on a wsn430-v1_3b board in the FIT/IoT-Lab:

That might be one of the last experiment on these devices: they are going to be removed during the summer :(

fjmolinas · 2020-06-26T10:36:59Z

cpu/msp430_common/include/irq_arch.h

+extern "C" {
+#endif
+
+extern volatile int __irq_is_in;


why is volatile the best choice here?

IMO it should not be volatile. But I didn't want to change this, as this not directly related to the API transition.

kaspar030 · 2020-06-26T10:39:43Z

I think this one will conflict a lot with #12457. That'll have to wait until I'm back from vacation, but IMO it would make sense to base this PR on that.

fjmolinas · 2020-06-26T10:46:34Z

I think this one will conflict a lot with #12457. That'll have to wait until I'm back from vacation, but IMO it would make sense to base this PR on that.

Do the instructions for isr handling change? Where do you see the issues exactly?

maribu · 2020-06-26T11:00:03Z

I think this one will conflict a lot with #12457.

I just rebased #12457 on top of this PR. These were the conflicts:

cpu/msp430_common/include/stdatomic.h
boards/chronos/doc.txt
boards/chronos/Kconfig
tests/gnrc_ipv6_nib/Makefile.ci
sys/net/gnrc/transport_layer/tcp/gnrc_tcp_pkt.c

None of them were caused by this PR. (And not was terrible difficult to fix; the first three files have just be removed, the Makefile.ci was trivial, and the gnrc_tcp_pkt.c was just the replacement of abs() with labs(), which can easily be redone on top of the new code.)

kaspar030 · 2020-06-26T11:01:12Z

I just rebased #12457 on top of this PR.

thanks for checking!

maribu · 2020-06-26T11:03:42Z

Anyway, soft feature freeze in effect since 20 minutes. So I guess this will have to wait until next release :-/

fjmolinas · 2020-06-26T11:15:45Z

Results:

Considering #13267 I think failures are OK!

fjmolinas · 2020-06-26T12:14:39Z

I'm not familiar with the msp430 instruction set, I will need to read on it a bit. But since feautre freeze is in, there is no real urgency :)

fjmolinas

I've looked at the assembler code and from my understanding looks OK, I had already tested before. With your change there is no usage of __enable_irq() and __disable_irq() is only used in thread_yield_higher, I think those private functions should be removed from cpu/msp430_common/include/cpu.h and the occurrence of __disable_irq replaced by irq_disable().

fjmolinas · 2020-07-15T10:05:06Z

I've looked at the assembler code and from my understanding looks OK, I had already tested before. With your change there is no usage of __enable_irq() and __disable_irq() is only used in thread_yield_higher, I think those private functions should be removed from cpu/msp430_common/include/cpu.h and the occurrence of __disable_irq replaced by irq_disable().

If you agree that those function should be removed, then please trigger murdock afterwards @maribu!

fjmolinas · 2020-07-15T10:23:12Z

@maribu Maybe remove those functions in a distinct commit instead of a fixup? We can wait for murdock to finish for that to see if other things pop up.

maribu · 2020-07-15T10:38:01Z

Some Murdock workers seem to have issues with the network file system right now :-/ Maybe someone from the CI team can take a look?

Drop `__enable_irq()` and `__disable_irq()` and replace single remaining call of them with the standard IRQ API, as this is now equally fast.

fjmolinas · 2020-07-15T11:28:27Z

@riot-ci @RIOTCI @aabadie @kaspar030 @cgundogan any ideas what is going on with murdock?

cgundogan · 2020-07-15T11:45:54Z

@riot-ci @RIOTCI @aabadie @kaspar030 @cgundogan any ideas what is going on with murdock?

there was a test server running out of memory. That is removed now from the build queue. I retriggered the affected PRs.

maribu · 2020-07-15T12:48:38Z

@cgundogan: Thanks! :-)

All green now :-)

fjmolinas

ACK!

maribu · 2020-07-15T13:47:33Z

Thanks :-)

maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: MSP Platform: This PR/issue effects MSP-based platforms Area: cpu Area: CPU/MCU ports labels Jun 25, 2020

maribu requested review from gschorcht and kaspar030 as code owners June 25, 2020 19:54

maribu mentioned this pull request Jun 25, 2020

[tracking] Transition to inline-able IRQ API #14356

Closed

7 tasks

fjmolinas reviewed Jun 26, 2020

View reviewed changes

fjmolinas reviewed Jul 15, 2020

View reviewed changes

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 15, 2020

cpu/msp430_common: Refactor cpu.{c,h}

1a8defd

Drop `__enable_irq()` and `__disable_irq()` and replace single remaining call of them with the standard IRQ API, as this is now equally fast.

maribu force-pushed the msp430-irq-inline branch from e87991b to 1a8defd Compare July 15, 2020 11:10

cgundogan 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 Jul 15, 2020

fjmolinas approved these changes Jul 15, 2020

View reviewed changes

fjmolinas merged commit 1167867 into RIOT-OS:master Jul 15, 2020

maribu deleted the msp430-irq-inline branch July 15, 2020 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/msp430_common: Update to inline-able IRQ API #14362

cpu/msp430_common: Update to inline-able IRQ API #14362

maribu commented Jun 25, 2020

fjmolinas commented Jun 26, 2020

aabadie commented Jun 26, 2020

fjmolinas Jun 26, 2020

maribu Jun 26, 2020

kaspar030 commented Jun 26, 2020

fjmolinas commented Jun 26, 2020

maribu commented Jun 26, 2020

kaspar030 commented Jun 26, 2020

maribu commented Jun 26, 2020

fjmolinas commented Jun 26, 2020

fjmolinas commented Jun 26, 2020

fjmolinas left a comment

fjmolinas commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

maribu commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

cgundogan commented Jul 15, 2020

maribu commented Jul 15, 2020

fjmolinas left a comment

maribu commented Jul 15, 2020

cpu/msp430_common: Update to inline-able IRQ API #14362

cpu/msp430_common: Update to inline-able IRQ API #14362

Conversation

maribu commented Jun 25, 2020

Contribution description

Testing procedure

tests/irq_disable_restore

tests/tests/bench_mutex_pingpong

Issues/PRs references

fjmolinas commented Jun 26, 2020

aabadie commented Jun 26, 2020

fjmolinas Jun 26, 2020

Choose a reason for hiding this comment

maribu Jun 26, 2020

Choose a reason for hiding this comment

kaspar030 commented Jun 26, 2020

fjmolinas commented Jun 26, 2020

maribu commented Jun 26, 2020

kaspar030 commented Jun 26, 2020

maribu commented Jun 26, 2020

fjmolinas commented Jun 26, 2020

fjmolinas commented Jun 26, 2020

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

maribu commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

cgundogan commented Jul 15, 2020

maribu commented Jul 15, 2020

fjmolinas left a comment

Choose a reason for hiding this comment

maribu commented Jul 15, 2020

`tests/irq_disable_restore`

`tests/tests/bench_mutex_pingpong`