xtimer: xtimer_now() not ISR safe

[kaspar030]

On platforms where xtimer is using a <32bit counter, xtimer_now() might cause a wrong time to be returned when called from ISRs, as the _high_count and _long_count interrupts might be missed (delayed). This is also true for xtimer_now64() for all platforms, and also for all calls while IRQ's are disabled.

Unfortunately this also affects all xtimer_set*(), as they call xtimer_now[64}().

Can anyone think of an algorithm to prevent this?

(something like, a) set a flag after e.g., half the underlying timer's tick count, b) unset on overflow, and if in isr the tick count is < half and the flag is not set, add one to the long counter...)

[OlegHahm]

As I'm too lazy to look it up and cannot remember, can you quickly confirm that this bug would lead to a timer value that might be off by 2^32 ticks (if _high_cnt is wrong) in the worst case?

[jnohlgard]

@OlegHahm When this occurs the timer would be one lltimer period off if I understand the problem correctly. One lltimer period is the period of the underlying hardware timer.

[OlegHahm]

@gebart, thanks, yes I meant 2^TIMER_WIDTH instead of 2^32.

[jnohlgard]

How about this for a start:
if we mask the hardware counter to use all bits except the MSB then we can handle the situation through simple addition of the lltimer now value and the high cnt, and use the current high_cnt as an accumulator variable. It will take some effort to rework parts of xtimer though.

[kaspar030]

@gebart I don't quite understand. We're already adding lltimer_now to high_cnt. how does masking the MSB help?

[jnohlgard]

@kaspar030 I need to structure my ideas further, but I have some more urgent things that I need to finish first. Anyway, the general idea is set the _high_cnt tick timeout to half a period instead of a full period and let the lltimer msb and the _high_cnt lsb overlap in some way, to catch any IRQ delay which is less than a half lltimer period.

[malosek]

Hello,
IMHO: with current xtimer architecture there does not seem to be solution to cover all the use cases.
what about to decrease xtimer resolution to ms? and use hw timer "just" for increasing variable on overflow (this seems to be necessary anyway)?
with such setup low freq clock could be used while high speed clock is off.
if there is need, then introduce kind of high speed timer with sub ms resolution afterwards.
btw: good catch @kaspar030
wbr
malo

[OlegHahm]

If one configures the timer on iotlab-m3 to run slower (e.g. by setting XTIMER_SHIFT to 3) pinging between nodes with a bigger payload show some unexpected behavior. Usually pinging between two 6lo nodes and a payload of 1024 bytes, has a round-trip time of about 150ms. With the slower timer, the RTT increases for some pings (~25%) to ~650ms while the rest shows the usual 150ms.

[kYc0o]

I need to postpone this until all related XTIMER PRs are solved.

[cladmi]

It can fail from both interrupts and masked interrupts. Even reading time from an xtimer callback can fail.

For me this is the first thing to solve. Other issues with concurrency issues checking XTIMER_BACKOFF cannot work if you cannot rely on reading time from masked interrupt context.

#5338 (comment) is indeed a way of handling it.

Another solution I imagine, would be to save the previous last time that was read and adjust the time returned by xtimer_now and xtimer_now64 to be sure it is not in the past. It should be updated once per period at least by the xtimer interrupt so would be detectable.

There is a maximum time that interrupt should be allowed to be masked anyway.
If it is more than a cycle, it cannot be detected. With half a cycle it should be able to work.

Note that there is already a usage that reads from masked interrupt and it fails for arduino-mega2560
#8040
RIOT-OS/Release-Specs#98

[cladmi]

Another solution I imagine, would be to save the previous last time that was read and adjust the time returned by xtimer_now and xtimer_now64 to be sure it is not in the past. It should be updated once per period at least by the xtimer interrupt so would be detectable.

Hmm, would not work as we are tickless. I was thinking it would be read sometime anyway, but you could spend the whole cycle and have masked interrupt just at the time of the overflow. So forcing an interrupt in the middle would be the similar to ticking twice per cycle.

[kaspar030]

@gebart and I solved this in ztimer (by ticking twice per cycle). I'll focus on getting that into shape when the SUIT stuff is finally merged. I hope ztimer will completely replace xtimer.

[cladmi]

@gebart and I solved this in ztimer (by ticking twice per cycle). I'll focus on getting that into shape when the SUIT stuff is finally merged. I hope ztimer will completely replace xtimer.

Any change required for doing a thread/interrupt/concurrent safe timer extension in xtimer would be required for ztimer.
Why not fix them first instead of replacing everything at once? It is change all with a 3k pull request and API change or nothing.

Also, most of the issues I saw on xtimer were there on day one. The issues were not spotted during the initial review and many never got fixed or got potential fixes reviewed and merged. With this in mind, I do not see why replacing the implementation will magically get a working maintainable version that solves all the problems at once.

I would be in favor of solving the issues first. This issue is already 2 years old.

[kaspar030]

With this in mind, I do not see why replacing the implementation will magically get a working maintainable version that solves all the problems at once.

Probably not, but ztimer has a modular design, and things like extending a timer from 16 to 32 bit is done in a module. There's also a mock timer that's used for unittesting these modules. So I expect a much better day-1 experience.

Anyhow, I don't want to stop you from fixing anything. ztimer won't be the default timer for quite a while, and any test cases you come up with will be useful to ztimer as well.

[MichelRottleuthner]

@fjmolinas answering your question from the other PR here:

Is that also the cas for #5338? Could you take a look at the issues that where fixed with #9530?

Yes this was also fixed by #9530, which can be proved with the test provided in #13025.
There are probably many more xtimer related issues/PRs that are now either fixed or obsolete.
I'll allocate some time tomorrow to sweep thru them.