[Qemu-devel] [PATCH, RFC] qemu-timer: fix alarm_timer pending

[Qemu-devel] [PATCH, RFC] qemu-timer: fix alarm_timer pending

[TeLeMan]

I fetched the lastest qemu-timer codes and found qemu would have no
response when the guest os was WinXP and the timer was "dynticks" on
the win32 host.  After qemu froze, it seemed the win32_rearm_timer()
would never be called and alarm_timer->pending was always 0.
I could not find the more deeper reason and just referred to the
previous implement to make this patch.

Signed-off-by: TeLeMan <geleman@gmail.com>
---
 qemu-timer.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 3d6e99a..f9e2b19 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -280,7 +280,12 @@ static struct qemu_alarm_timer *alarm_timer;

 int qemu_alarm_pending(void)
 {
-    return alarm_timer->pending;
+    if(alarm_timer->pending) {
+        alarm_timer->pending = 0;
+        return 1;
+    }
+
+    return 0;
 }

 static inline int alarm_has_dynticks(struct qemu_alarm_timer *t)
@@ -724,8 +729,6 @@ void qemu_run_all_timers(void)
         qemu_rearm_alarm_timer(alarm_timer);
     }

-    alarm_timer->pending = 0;
-
     /* vm time timers */
     if (vm_running) {
         qemu_run_timers(vm_clock);
-- 
1.6.5.1.1367.gcd48
--
SUN OF A BEACH

[Qemu-devel] Re: [PATCH, RFC] qemu-timer: fix alarm_timer pending

[Paolo Bonzini]

On 03/19/2010 06:24 AM, TeLeMan wrote:
> I fetched the lastest qemu-timer codes and found qemu would have no
> response when the guest os was WinXP and the timer was "dynticks" on
> the win32 host.  After qemu froze, it seemed the win32_rearm_timer()
> would never be called and alarm_timer->pending was always 0.
> I could not find the more deeper reason and just referred to the
> previous implement to make this patch.

Interesting, it ran fine for me under Wine.

I can see why the patch you have works, but I don't think it's 100% 
correct.  alarm_timer->pending should remain 1 until qemu_run_all_timers 
runs.  Can you test this one instead:

diff --git a/qemu-timer.c b/qemu-timer.c
index 329d3a4..49eac86 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -706,14 +706,14 @@ void configure_icount(const char *option)

  void qemu_run_all_timers(void)
  {
+    alarm_timer->pending = 0;
+
      /* rearm timer, if not periodic */
      if (alarm_timer->expired) {
          alarm_timer->expired = 0;
          qemu_rearm_alarm_timer(alarm_timer);
      }

-    alarm_timer->pending = 0;
-
      /* vm time timers */
      if (vm_running) {
          qemu_run_timers(vm_clock);


If it doesn't work, I'm fine with TeLeMan's patch.

Paolo

[Qemu-devel] Re: [PATCH, RFC] qemu-timer: fix alarm_timer pending

[TeLeMan]

On Fri, Mar 19, 2010 at 17:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 03/19/2010 06:24 AM, TeLeMan wrote:
>>
>> I fetched the lastest qemu-timer codes and found qemu would have no
>> response when the guest os was WinXP and the timer was "dynticks" on
>> the win32 host.  After qemu froze, it seemed the win32_rearm_timer()
>> would never be called and alarm_timer->pending was always 0.
>> I could not find the more deeper reason and just referred to the
>> previous implement to make this patch.
>
> Interesting, it ran fine for me under Wine.
>
> I can see why the patch you have works, but I don't think it's 100% correct.
>  alarm_timer->pending should remain 1 until qemu_run_all_timers runs.  Can
> you test this one instead:

Yes, I did test this one firstly. It's working also but different with
the previous implement and I didn't know the exact reason, so I made
the another patch.


> diff --git a/qemu-timer.c b/qemu-timer.c
> index 329d3a4..49eac86 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -706,14 +706,14 @@ void configure_icount(const char *option)
>
>  void qemu_run_all_timers(void)
>  {
> +    alarm_timer->pending = 0;
> +
>     /* rearm timer, if not periodic */
>     if (alarm_timer->expired) {
>         alarm_timer->expired = 0;
>         qemu_rearm_alarm_timer(alarm_timer);
>     }
>
> -    alarm_timer->pending = 0;
> -
>     /* vm time timers */
>     if (vm_running) {
>         qemu_run_timers(vm_clock);
>
>
> If it doesn't work, I'm fine with TeLeMan's patch.
>
> Paolo
>

[Qemu-devel] [PATCH] fix race between timer firing vs. alarm_timer->pending = 0

[Paolo Bonzini]

The period for Win32 timers is very short and always the same
independent of dynticks, so it's possible that the timer fires
before qemu_run_all_timers has reset alarm_timer->pending to zero.
Reset alarm_timer->pending before rearming.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-timer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 329d3a4..49eac86 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -706,14 +706,14 @@ void configure_icount(const char *option)
 
 void qemu_run_all_timers(void)
 {
+    alarm_timer->pending = 0;
+
     /* rearm timer, if not periodic */
     if (alarm_timer->expired) {
         alarm_timer->expired = 0;
         qemu_rearm_alarm_timer(alarm_timer);
     }
 
-    alarm_timer->pending = 0;
-
     /* vm time timers */
     if (vm_running) {
         qemu_run_timers(vm_clock);
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH] fix race between timer firing vs. alarm_timer->pending = 0

[Aurelien Jarno]

On Fri, Mar 19, 2010 at 11:30:35AM +0100, Paolo Bonzini wrote:
> The period for Win32 timers is very short and always the same
> independent of dynticks, so it's possible that the timer fires
> before qemu_run_all_timers has reset alarm_timer->pending to zero.
> Reset alarm_timer->pending before rearming.

Thanks, applied.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-timer.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-timer.c b/qemu-timer.c
> index 329d3a4..49eac86 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -706,14 +706,14 @@ void configure_icount(const char *option)
>  
>  void qemu_run_all_timers(void)
>  {
> +    alarm_timer->pending = 0;
> +
>      /* rearm timer, if not periodic */
>      if (alarm_timer->expired) {
>          alarm_timer->expired = 0;
>          qemu_rearm_alarm_timer(alarm_timer);
>      }
>  
> -    alarm_timer->pending = 0;
> -
>      /* vm time timers */
>      if (vm_running) {
>          qemu_run_timers(vm_clock);
> -- 
> 1.6.6.1
> 
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net