Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xtimer: periodic race #9595

Closed
wants to merge 1 commit into from
Closed

Conversation

Josar
Copy link
Contributor

@Josar Josar commented Jul 18, 2018

This PR solves a race condition.

If the workload is high like in test/xtimer_drift and the last_wakeup is not updated with the proper time the next target might be also allready past now, dependend on the periode choosen.

This PR sets the last_wakeup to now if the target is allready reached.

EDIT:
I change the implementation so the past targets are skipped and the period is fixed.
Means past targets get skipped, one execution with hight jitter, next execution in periode with minimal jitter.

Tries to handle the same problem as #7629 submitted on 20 Sep 2017.

@PeterKietzmann PeterKietzmann requested a review from kaspar030 July 23, 2018 08:08
@PeterKietzmann PeterKietzmann added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: timers Area: timer subsystems labels Jul 23, 2018
@PeterKietzmann PeterKietzmann requested a review from cladmi July 23, 2018 08:09
@kaspar030
Copy link
Contributor

kaspar030 commented Jul 23, 2018

Hm, now the period drifts on underflow, right? E.g., start is 0, period is 10, if one step at N*10 is missed by M, from that point on the timer wakes up at (N*10 + M), thus the period moved.

@Josar
Copy link
Contributor Author

Josar commented Jul 23, 2018

@kaspar030 thats correct.

If this happens this means there are to much higher prior task, or they take to long, and the period can not be fullfilled. It would be cancelled out if this is only one period with high load. then the next target will be on spot again.

But if it happpens every 10 period and If the period does not move the target will get closer to now.
At some point, If the higher prior task needs longer than the period, there will be mulltiple underflows. Now if the higher prior task, as in xtimer_drift, depends on the xtimer_periodic_wakeup() it will be triggered faster than expected as there is work to compensate. As the higher prior task will now again need multiple periods the target will always be smaller than know, race condition.

grafik

with this fix there will be a continues growing drift from the starting target, but the it will keep running.

grafik

@kaspar030
Copy link
Contributor

I guess this is a matter of expected / desired behaviour.

  1. if the wakeup function realizes it is late, is it OK to skip one or more "ticks", or should there be one wakeup for each missed tick, even if they'd happen right away?
  2. if yes, trigger one ASAP or at the next planned "tick"?
  3. if ASAP, from then on, shift the period or try to get back to the original period (set (last = now) or last = (expcted + period)?

What would we expect?

@Josar
Copy link
Contributor Author

Josar commented Jul 23, 2018

@kaspar030 this was the first fix.

But for sure skipping and setting the next target in regard to the old target is a possible solution which would not introduce a jitter but only a drift.

@Josar
Copy link
Contributor Author

Josar commented Jul 25, 2018

I change the implementation so the past targets are skipped and the period is fixed.

@Josar Josar force-pushed the pr/xtimer_periodic_race branch from dbe13f2 to 4cdb29e Compare July 26, 2018 11:42
@Hyungsin
Copy link

Hyungsin commented Aug 9, 2018

@Josar, glad to find another person working on xtimer. I also opened a PR regarding xtimer implementation for eliminating race conditions in xtimer #9530.

@Josar
Copy link
Contributor Author

Josar commented Sep 18, 2018

@kaspar030 @cladmi @gebart What would be the desired behaviour in your opinion?
What do you think about this approach?

@Josar Josar mentioned this pull request Nov 15, 2018
@Josar Josar force-pushed the pr/xtimer_periodic_race branch 3 times, most recently from 3518b88 to a5bf1cc Compare November 15, 2018 18:04
1. Change targed passed criteria.
2. Recalculate last_wakeup to be a multiple of periods after now.
   This skips missed targets and avoids a race condition.
@Josar Josar force-pushed the pr/xtimer_periodic_race branch from a5bf1cc to 01db77e Compare November 15, 2018 18:05
@Josar
Copy link
Contributor Author

Josar commented Nov 15, 2018

Is there any chance to work on this PR. As this handles a problem which is idling around quite long.
See the solution of @gebart from 2017 84f42e2

@Josar
Copy link
Contributor Author

Josar commented Dec 18, 2018

@gebart and @miri64 as you have worked on #7629, which tries to solve the same issue, could you have a look at this?

@Josar
Copy link
Contributor Author

Josar commented Jan 30, 2019

Ping Pong

@Josar
Copy link
Contributor Author

Josar commented Mar 17, 2019

@kYc0o @kaspar030 @cladmi @gebart seems there are still people having problems with this issue. Maybe someone could have time to look into this PR.

@stale
Copy link

stale bot commented Sep 18, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Sep 18, 2019
@stale stale bot closed this Oct 19, 2019
@miri64 miri64 reopened this Oct 19, 2019
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Oct 19, 2019
@miri64
Copy link
Member

miri64 commented Jun 23, 2020

@MichelRottleuthner @JulianHolzwarth can you have a look if this PR is still valid after the xtimer rework?

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@kaspar030
Copy link
Contributor

I'm closing this as xtimer is deprecated. Feel free to reopen if you disagree.

@kaspar030 kaspar030 closed this Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: timers Area: timer subsystems Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants