CP-52821: use Mtime in Xapi_periodic_scheduler

[edwintorok]

Target: feature/perf

Note that this conflicts with 7e9310f?diff=unified&w=1, but is a pre-requisite of #6126

[robhoes]

Isn't it better to rebase on master already and build on top of the scheduler changes there? Those rebase changes would have to be done later anyway, but then end up in a complicated and hard to review merge commit.

[psafont]

There's a further improvement here: replace all Mtime_clock.now () with Mtime_clock.elapsed() to remove the overflow treatment because the latter returns a span instead of a timestamp

[edwintorok]

Isn't it better to rebase on master already and build on top of the scheduler changes there? Those rebase changes would have to be done later anyway, but then end up in a complicated and hard to review merge commit.

I'll rebase this PR on top of master once Frediano's PR is merged.

[freddy77]

There's a further improvement here: replace all Mtime_clock.now () with Mtime_clock.elapsed() to remove the overflow treatment because the latter returns a span instead of a timestamp

It's not clear what overflows you are talking about. Internally the timer is a counter since boot, that will overflow the int64 (although the numbers should be unsigned) in about 290 years. I personally find the code with Mtime_clock.elapsed() less readable having to deal with differences of differences (this span is less or more than this other span) instead of just comparing time (the deadline cross current time).

PS: @psafont are you referring to floating point? Maybe it would be sensible to consider a too big timeout (like 10 years) as an error?

[psafont]

It's not clear what overflows you are talking about.

One function returns an option in case of overflow, the other does not. Because, as you point out, the overflow won't happen in practise, I prefer to use the latter rather than the former.

I personally find the code with Mtime_clock.elapsed() less readable having to deal with differences of differences

I fundamentally disagree, timestamps are actually differences as well (between an epoch and a point in time), and the only practical difference is replacing is_{earlier,later} with is_{shorter,longer}. I'm fine with the improvement not being in this PR, it was only an observation

[edwintorok]

Moved this to draft until the referenced PR is merged to master as this will need a rebase. #6155

[edwintorok]

Rebased on latest feature/perf (which in turn has been updated to include latest master).
Target is still feature/perf because there are more changes coming, and this needs a bit wider testing.

E.g. previously there was a +0.5 in the periodic scheduler, but now we just use Clock.Timer.remaining, so there could be some subtle differences in behaviour.

[edwintorok]

Rebased on latest feature/perf which incorporated the latest changes to the scheduler from master.

[freddy77]

"Minor" hidden change. Due to internal implementation moving from Mtime_clock.now to Mtime_clock.elapsed on Linux move the timer from CLOCK_MONOTONIC to CLOCK_BOOTTIME. They are both monotonic but the second takes into account when the machine is suspended. But in our case machine should not be suspended so it should be the same.

[edwintorok]

move the timer from CLOCK_MONOTONIC to CLOCK_BOOTTIME

For something like 'run this task every 24h' that actually seems like the correct behaviour (even if the machine was asleep we'd want it to run the task if it wakes up sooner than that), so the periodic tasks should probably use BOOTTIME (i.e. Mtime_clock.elapsed), and oneshot tasks monotonic (i.e. Mtime_clock.now).

But as you said we don't actually suspend Dom0, so we don't need to complicate the scheduler to make that distinction.