util/qemu-timer.c: Don't warp timer from timerlist_rearm()

Currently we call icount_start_warp_timer() from timerlist_rearm().
This produces incorrect behaviour, because timerlist_rearm() is
called, for instance, when a timer callback modifies its timer.  We
cannot decide here to warp the timer forwards to the next timer
deadline merely because all_cpu_threads_idle() is true, because the
timer callback we were called from (or some other callback later in
the list of callbacks being invoked) may be about to raise a CPU
interrupt and move a CPU from idle to ready.5A

The only valid place to choose to warp the timer forward is from the
main loop, when we know we have no outstanding IO or timer callbacks
that might be about to wake up a CPU.

For Arm guests, this bug was mostly latent until the refactoring
commit f6fc36deef6abc ("target/arm/helper: Implement
CNTHCTL_EL2.CNT[VP]MASK"), which exposed it because it refactored a
timer callback so that it happened to call timer_mod() first and
raise the interrupt second, when it had previously raised the
interrupt first and called timer_mod() afterwards.

This call seems to have originally derived from the
pre-record-and-replay icount code, which (as of e.g.  commit
db1a49726c3c in 2010) in this location did a call to
qemu_notify_event(), necessary to get the icount code in the vCPU
round-robin thread to stop and recalculate the icount deadline when a
timer was reprogrammed from the IO thread.  In current QEMU,
everything is done on the vCPU thread when we are in icount mode, so
there's no need to try to notify another thread here.

I suspect that the other reason why this call was doing icount timer
warping is that it pre-dates commit efab87cf79077a from 2015, which
added a call to icount_start_warp_timer() to main_loop_wait().  Once
the call in timerlist_rearm() has been removed, if the timer
callbacks don't cause any CPU to be woken up then we will end up
calling icount_start_warp_timer() from main_loop_wait() when the rr
main loop code calls rr_wait_io_event().

Remove the incorrect call from timerlist_rearm().

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
As far as I can tell, this is the right thing, and it fixes the
"icount warps the timer when it should not" bug I'm trying to
address, but I'm not super familiar with the icount subsystem or its
evolution, so it's possible I've accidentally broken some other setup
here.  This does pass the tcg, functional and avocado tests,
including the record-and-replay ones.  I've cc'd it to stable as a
bugfix, but it definitely merits careful review first.
---
 util/qemu-timer.c | 4 ----
 1 file changed, 4 deletions(-)

On Mon, 10 Feb 2025 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Currently we call icount_start_warp_timer() from timerlist_rearm().
> This produces incorrect behaviour, because timerlist_rearm() is
> called, for instance, when a timer callback modifies its timer.  We
> cannot decide here to warp the timer forwards to the next timer
> deadline merely because all_cpu_threads_idle() is true, because the
> timer callback we were called from (or some other callback later in
> the list of callbacks being invoked) may be about to raise a CPU
> interrupt and move a CPU from idle to ready.5A

(oops, stray editor damage "5A" at end of line)

>
> The only valid place to choose to warp the timer forward is from the
> main loop, when we know we have no outstanding IO or timer callbacks
> that might be about to wake up a CPU.

This raises actually another question: should the call to
icount_start_warp_timer() in main_loop_wait() maybe go after
qemu_clock_run_all_timers() rather than before? (Haven't tested
whether that breaks anything ;-))

-- PMM

On Mon, 10 Feb 2025 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Currently we call icount_start_warp_timer() from timerlist_rearm().
> This produces incorrect behaviour, because timerlist_rearm() is
> called, for instance, when a timer callback modifies its timer.  We
> cannot decide here to warp the timer forwards to the next timer
> deadline merely because all_cpu_threads_idle() is true, because the
> timer callback we were called from (or some other callback later in
> the list of callbacks being invoked) may be about to raise a CPU
> interrupt and move a CPU from idle to ready.5A
>
> The only valid place to choose to warp the timer forward is from the
> main loop, when we know we have no outstanding IO or timer callbacks
> that might be about to wake up a CPU.

This patch also seems to fix
https://gitlab.com/qemu-project/qemu/-/issues/2703
(a report that with an x86 system sometimes the ptimer
period would be longer than you'd programmed the ptimer for).

thanks
-- PMM

On 2/10/25 05:58, Peter Maydell wrote:
> Currently we call icount_start_warp_timer() from timerlist_rearm().
> This produces incorrect behaviour, because timerlist_rearm() is
> called, for instance, when a timer callback modifies its timer.  We
> cannot decide here to warp the timer forwards to the next timer
> deadline merely because all_cpu_threads_idle() is true, because the
> timer callback we were called from (or some other callback later in
> the list of callbacks being invoked) may be about to raise a CPU
> interrupt and move a CPU from idle to ready.5A
> 
> The only valid place to choose to warp the timer forward is from the
> main loop, when we know we have no outstanding IO or timer callbacks
> that might be about to wake up a CPU.
> 
> For Arm guests, this bug was mostly latent until the refactoring
> commit f6fc36deef6abc ("target/arm/helper: Implement
> CNTHCTL_EL2.CNT[VP]MASK"), which exposed it because it refactored a
> timer callback so that it happened to call timer_mod() first and
> raise the interrupt second, when it had previously raised the
> interrupt first and called timer_mod() afterwards.
> 
> This call seems to have originally derived from the
> pre-record-and-replay icount code, which (as of e.g.  commit
> db1a49726c3c in 2010) in this location did a call to
> qemu_notify_event(), necessary to get the icount code in the vCPU
> round-robin thread to stop and recalculate the icount deadline when a
> timer was reprogrammed from the IO thread.  In current QEMU,
> everything is done on the vCPU thread when we are in icount mode, so
> there's no need to try to notify another thread here.
> 
> I suspect that the other reason why this call was doing icount timer
> warping is that it pre-dates commit efab87cf79077a from 2015, which
> added a call to icount_start_warp_timer() to main_loop_wait().  Once
> the call in timerlist_rearm() has been removed, if the timer
> callbacks don't cause any CPU to be woken up then we will end up
> calling icount_start_warp_timer() from main_loop_wait() when the rr
> main loop code calls rr_wait_io_event().
> 
> Remove the incorrect call from timerlist_rearm().
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> As far as I can tell, this is the right thing, and it fixes the
> "icount warps the timer when it should not" bug I'm trying to
> address, but I'm not super familiar with the icount subsystem or its
> evolution, so it's possible I've accidentally broken some other setup
> here.  This does pass the tcg, functional and avocado tests,
> including the record-and-replay ones.  I've cc'd it to stable as a
> bugfix, but it definitely merits careful review first.
> ---

I don't completely grok icount either, but this patch at least conforms to my 
expectations.  The fact that it passes record/replay is a really good smoke test.

So fwiw,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

On 2/10/25 06:08, Peter Maydell wrote:
> On Mon, 10 Feb 2025 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> Currently we call icount_start_warp_timer() from timerlist_rearm().
>> This produces incorrect behaviour, because timerlist_rearm() is
>> called, for instance, when a timer callback modifies its timer.  We
>> cannot decide here to warp the timer forwards to the next timer
>> deadline merely because all_cpu_threads_idle() is true, because the
>> timer callback we were called from (or some other callback later in
>> the list of callbacks being invoked) may be about to raise a CPU
>> interrupt and move a CPU from idle to ready.5A
> 
> (oops, stray editor damage "5A" at end of line)
> 
>>
>> The only valid place to choose to warp the timer forward is from the
>> main loop, when we know we have no outstanding IO or timer callbacks
>> that might be about to wake up a CPU.
> 
> This raises actually another question: should the call to
> icount_start_warp_timer() in main_loop_wait() maybe go after
> qemu_clock_run_all_timers() rather than before? (Haven't tested
> whether that breaks anything ;-))

It seems likely.  :-)


r~

On Mon, 10 Feb 2025 at 14:08, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 10 Feb 2025 at 13:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > Currently we call icount_start_warp_timer() from timerlist_rearm().
> > This produces incorrect behaviour, because timerlist_rearm() is
> > called, for instance, when a timer callback modifies its timer.  We
> > cannot decide here to warp the timer forwards to the next timer
> > deadline merely because all_cpu_threads_idle() is true, because the
> > timer callback we were called from (or some other callback later in
> > the list of callbacks being invoked) may be about to raise a CPU
> > interrupt and move a CPU from idle to ready.5A
>
> (oops, stray editor damage "5A" at end of line)
>
> >
> > The only valid place to choose to warp the timer forward is from the
> > main loop, when we know we have no outstanding IO or timer callbacks
> > that might be about to wake up a CPU.
>
> This raises actually another question: should the call to
> icount_start_warp_timer() in main_loop_wait() maybe go after
> qemu_clock_run_all_timers() rather than before? (Haven't tested
> whether that breaks anything ;-))

After further thought, I don't think this matters. If we
call warp_timer before the run_all_timers callback then
the warp will only warp the timer forward to at most the
earliest timer deadline (and then run_all_timers will
execute it); if there's already a timer whose deadline
has expired then warp_timer won't do anything.
If we call warp_timer after the run_all_timers callback
then I think the only difference is that we would go
round the loop another time before executing the
timer callback. Either way, that bit of the code doesn't
need changing.

-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> Currently we call icount_start_warp_timer() from timerlist_rearm().
> This produces incorrect behaviour, because timerlist_rearm() is
> called, for instance, when a timer callback modifies its timer.  We
> cannot decide here to warp the timer forwards to the next timer
> deadline merely because all_cpu_threads_idle() is true, because the
> timer callback we were called from (or some other callback later in
> the list of callbacks being invoked) may be about to raise a CPU
> interrupt and move a CPU from idle to ready.5A
>
> The only valid place to choose to warp the timer forward is from the
> main loop, when we know we have no outstanding IO or timer callbacks
> that might be about to wake up a CPU.
>
> For Arm guests, this bug was mostly latent until the refactoring
> commit f6fc36deef6abc ("target/arm/helper: Implement
> CNTHCTL_EL2.CNT[VP]MASK"), which exposed it because it refactored a
> timer callback so that it happened to call timer_mod() first and
> raise the interrupt second, when it had previously raised the
> interrupt first and called timer_mod() afterwards.
>
> This call seems to have originally derived from the
> pre-record-and-replay icount code, which (as of e.g.  commit
> db1a49726c3c in 2010) in this location did a call to
> qemu_notify_event(), necessary to get the icount code in the vCPU
> round-robin thread to stop and recalculate the icount deadline when a
> timer was reprogrammed from the IO thread.  In current QEMU,
> everything is done on the vCPU thread when we are in icount mode, so
> there's no need to try to notify another thread here.
>
> I suspect that the other reason why this call was doing icount timer
> warping is that it pre-dates commit efab87cf79077a from 2015, which
> added a call to icount_start_warp_timer() to main_loop_wait().  Once
> the call in timerlist_rearm() has been removed, if the timer
> callbacks don't cause any CPU to be woken up then we will end up
> calling icount_start_warp_timer() from main_loop_wait() when the rr
> main loop code calls rr_wait_io_event().
>
> Remove the incorrect call from timerlist_rearm().
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> As far as I can tell, this is the right thing, and it fixes the
> "icount warps the timer when it should not" bug I'm trying to
> address, but I'm not super familiar with the icount subsystem or its
> evolution, so it's possible I've accidentally broken some other setup
> here.  This does pass the tcg, functional and avocado tests,
> including the record-and-replay ones.  I've cc'd it to stable as a
> bugfix, but it definitely merits careful review first.

Yeah as far as I can tell in all the situations we do this with icount
we should always be exiting the loop anyway. Changing our mind about
when the next time slice ends while in a block would break things. For
IO triggered events we already know the block is about to end anyway and
things be rectified.

I did wonder if pmuevents (under icount) might suffer because they call
timer_mod_anticipate_ns in pmccntr_op_finish but my kvm-unit-test smoke
test was fine.

So:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>


> ---
>  util/qemu-timer.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> index 0e8a453eaa1..af6e1787e57 100644
> --- a/util/qemu-timer.c
> +++ b/util/qemu-timer.c
> @@ -409,10 +409,6 @@ static bool timer_mod_ns_locked(QEMUTimerList *timer_list,
>  
>  static void timerlist_rearm(QEMUTimerList *timer_list)
>  {
> -    /* Interrupt execution to force deadline recalculation.  */
> -    if (icount_enabled() && timer_list->clock->type == QEMU_CLOCK_VIRTUAL) {
> -        icount_start_warp_timer();
> -    }
>      timerlist_notify(timer_list);
>  }