Optimize tickless tick computation

[c1728p9]

Description

Optimize the tick computation by computing the elapsed ticks based on relative time rather than absolute time. On the NUCLEO_L073RZ this reduces the execution time of SysTimer::resume from >35us to less than 15us 20us.

Also, to ensure the relative time calculations do not overflow set the max time the OS can suspend to 1 hour.

Pull request type

[x ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@pan-

[ciarmcom]

@c1728p9, thank you for your changes.
@pan- @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[kjbracey]

This saves a fair bit on the M0, where it converts a 64-bit software division to a 32-bit software division, but on M3 and M4 it should save a lot more - it should be optimised into a "multiply by reciprocal" using UMULL. (I know GCC and ARMC5 do this, hopefully IAR and ARMC6 do too).

This is slightly at odds with something I'm looking to do with SysTimer - that was actually going to make it more 64-bit, to help support a manual suspend API, where I was anticipating long sleep times. But I think I could deal with that by having conditional behaviour - switch between optimised 32-bit and heavy 64-bit computations depending on whether deep sleep is locked. If deep sleep is permitted, we have a free reign on interrupt latency.

[jeromecoutant]

@janjongboom
Maybe you can also have a look as I think you are using tickless for L072 Lora board ?

@LMESTM

[c1728p9]

I think this remainder calculation is wrong.

[c1728p9]

I updated this PR to preserver the long sleep times without having an impact on interrupt latency. Bringing back 64 bit division brought execution time of SysTimer::resume up to to a bit under 20us. I also added a separate commit to remove the critical section in places where it wasn't needed, such as SysTimer::resume.

[kjbracey]

Gah!!! I briefly wondered whether this might be a solution, but immediately shot it down because I assumed SysTimer::resume was being called in a critical section from outside. This is great.

I think it's still worth optimising for the common short case - if this is taking 20us every millisecond, that is 2% of CPU time just for the division. (On a 4MHz system I saw about 10% of CPU time being eaten up by the millisecond tickers.).

How about:

uint64_t elapsed_us = ticker_read_us(_ticker_data) - _time_us;
uint64_t elapsed_ticks;
if (elapsed_us > 0xFFFFFFFF) {
    // This will typically invoke full 64/64 software division
    elapsed_ticks = elapsed_us / US_IN_TICK;
} else {
    // This will will invoke 32/32 software division on ARMv6-M or ARMv8-M baseline, and
    // on ARMv7-M or ARMv8-M mainline can be optimised to a UMULL by 2^n/US_IN_TICK, or
    // at worst be a UDIV instruction. Thus it will be faster, maybe greatly.
    elapsed_ticks = (uint32_t) elapsed_us / US_IN_TICK;
}

[c1728p9]

I assumed SysTimer::resume was being called in a critical section from outside.

It used to be in a critical section, but had to be changed when the RTX API it relied on became unavailable. I missed it at this time as well.

Initially I had it avoid division all together in the fast case - when only a single tick had elapsed:

if (elapsed_time < US_IN_TICK) {
    elapsed_ticks = 0;
} else if (elapsed_time < US_IN_TICK * 2) {
    elapsed_ticks = 1;
} else {
    elapsed_ticks = elapsed_time / US_IN_TICK;
}

I ended up going with the simpler implementation you see now since I was just worried about reducing worst case execution time. Also, I was only setup to profile only worst-case execution time, which did not change much from this.

It sounds like you have profiled the average execution time on a couple of devices. Would you be able to profile the different optimizations and create a PR for the one which looks the most promising?

[kjbracey]

I'll have to see if those extra cases help too. My feeling is that division tends to have early termination, so might not help much, but worth checking.

I was just worried about reducing worst case execution time.

But we don't care about worst-case any more if we're outside of the critical section, right?

[c1728p9]

But we don't care about worst-case any more if we're outside of the critical section, right?

True, worst case doesn't matter anymore. At the time I was only setup to profile worst-case, so I wasn't able to evaluate the impact of this change.

[c1728p9]

Rebased to master and updated US_IN_TICK calculation/compiler assert.

[janjongboom]

@jeromecoutant I don't feel comfortable reviewing this at this level. But I trust @c1728p9

[cmonr]

CI started whilst waiting on reviews.

[mbed-ci]

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

[cmonr]

@mbed-os-core @pan- Thoguhts?