[Qemu-devel] [PATCH] audio: make audio poll timer deterministic

[Qemu-devel] [PATCH] audio: make audio poll timer deterministic

[Pavel Dovgalyuk]

This patch changes resetting strategy of the audio polling timer.
It does not change expiration time if the timer is already set.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 audio/audio.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index c845a44..1ee95a5 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1112,8 +1112,10 @@ static int audio_is_timer_needed (void)
 static void audio_reset_timer (AudioState *s)
 {
     if (audio_is_timer_needed ()) {
-        timer_mod (s->ts,
-            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
+        if (!timer_pending(s->ts)) {
+            timer_mod (s->ts,
+                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
+        }
     }
     else {
         timer_del (s->ts);

Re: [Qemu-devel] [PATCH] audio: make audio poll timer deterministic

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH] audio: make audio poll timer deterministic
Message-id: 20170131115913.6828.12266.stgit@PASHA-ISP

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20170131115913.6828.12266.stgit@PASHA-ISP -> patchew/20170131115913.6828.12266.stgit@PASHA-ISP
Switched to a new branch 'test'
9c7ac53 audio: make audio poll timer deterministic

=== OUTPUT BEGIN ===
Checking PATCH 1/1: audio: make audio poll timer deterministic...
ERROR: space prohibited between function name and open parenthesis '('
#23: FILE: audio/audio.c:1116:
+            timer_mod (s->ts,

total: 1 errors, 0 warnings, 12 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] audio: make audio poll timer deterministic

[Marc-André Lureau]

Hi

On Tue, Jan 31, 2017 at 4:03 PM Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
wrote:

> This patch changes resetting strategy of the audio polling timer.
> It does not change expiration time if the timer is already set.
>
>
You describe the change, but could you explain the reasons you propose it?


> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  audio/audio.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index c845a44..1ee95a5 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1112,8 +1112,10 @@ static int audio_is_timer_needed (void)
>  static void audio_reset_timer (AudioState *s)
>  {
>      if (audio_is_timer_needed ()) {
> -        timer_mod (s->ts,
> -            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
> +        if (!timer_pending(s->ts)) {
> +            timer_mod (s->ts,
> +                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> conf.period.ticks);
> +        }
>      }
>      else {
>          timer_del (s->ts);
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH] audio: make audio poll timer deterministic

[Pavel Dovgalyuk]

This patch is needed to make this timer deterministic and to use execution record/replay

for audio devices.

 

audio_reset_timer is used in the function audio_vm_change_state_handler.

Therefore every time VM is stopped or restarted the timer will be reset to new timeout.

Virtual clock does not proceed while VM is stopped. Therefore there is no need

in resetting the timeout when VM restarts.

 

Pavel Dovgalyuk

 

From: Marc-André Lureau [mailto:marcandre.lureau@gmail.com] 
Sent: Tuesday, January 31, 2017 3:16 PM
To: Pavel Dovgalyuk; qemu-devel@nongnu.org
Cc: pbonzini@redhat.com; dovgaluk@ispras.ru; kraxel@redhat.com
Subject: Re: [Qemu-devel] [PATCH] audio: make audio poll timer deterministic

 

Hi

 

On Tue, Jan 31, 2017 at 4:03 PM Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> wrote:

This patch changes resetting strategy of the audio polling timer.
It does not change expiration time if the timer is already set.

 

You describe the change, but could you explain the reasons you propose it?
 

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 audio/audio.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index c845a44..1ee95a5 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1112,8 +1112,10 @@ static int audio_is_timer_needed (void)
 static void audio_reset_timer (AudioState *s)
 {
     if (audio_is_timer_needed ()) {
-        timer_mod (s->ts,
-            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
+        if (!timer_pending(s->ts)) {
+            timer_mod (s->ts,
+                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
+        }
     }
     else {
         timer_del (s->ts);



-- 

Marc-André Lureau

Re: [Qemu-devel] [PATCH] audio: make audio poll timer deterministic

[Gerd Hoffmann]

On Di, 2017-01-31 at 15:32 +0300, Pavel Dovgalyuk wrote:
> This patch is needed to make this timer deterministic and to use
> execution record/replay
> 
> for audio devices.
> 
>  
> 
> audio_reset_timer is used in the function
> audio_vm_change_state_handler.
> 
> Therefore every time VM is stopped or restarted the timer will be
> reset to new timeout.
> 
> Virtual clock does not proceed while VM is stopped. Therefore there is
> no need
> 
> in resetting the timeout when VM restarts.

Add that to the commit message please.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] audio: make audio poll timer deterministic

[Pavel Dovgalyuk]

Ping?

Pavel Dovgalyuk


> -----Original Message-----
> From: Pavel Dovgalyuk [mailto:pavel.dovgaluk@ispras.ru]
> Sent: Tuesday, January 31, 2017 2:59 PM
> To: qemu-devel@nongnu.org
> Cc: pbonzini@redhat.com; dovgaluk@ispras.ru; kraxel@redhat.com
> Subject: [PATCH] audio: make audio poll timer deterministic
> 
> This patch changes resetting strategy of the audio polling timer.
> It does not change expiration time if the timer is already set.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  audio/audio.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index c845a44..1ee95a5 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1112,8 +1112,10 @@ static int audio_is_timer_needed (void)
>  static void audio_reset_timer (AudioState *s)
>  {
>      if (audio_is_timer_needed ()) {
> -        timer_mod (s->ts,
> -            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
> +        if (!timer_pending(s->ts)) {
> +            timer_mod (s->ts,
> +                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
> +        }
>      }
>      else {
>          timer_del (s->ts);

Re: [Qemu-devel] [PATCH] audio: make audio poll timer deterministic

[Yurii Zubrytskyi]

Hi,

It looks to me that this behavior can be achieved with "timer_mod_anticipate()"
function instead of a separate check.

On Sun, Feb 12, 2017 at 9:04 PM, Pavel Dovgalyuk <dovgaluk@ispras.ru> wrote:

> Ping?
>
> Pavel Dovgalyuk
>
>
> > -----Original Message-----
> > From: Pavel Dovgalyuk [mailto:pavel.dovgaluk@ispras.ru]
> > Sent: Tuesday, January 31, 2017 2:59 PM
> > To: qemu-devel@nongnu.org
> > Cc: pbonzini@redhat.com; dovgaluk@ispras.ru; kraxel@redhat.com
> > Subject: [PATCH] audio: make audio poll timer deterministic
> >
> > This patch changes resetting strategy of the audio polling timer.
> > It does not change expiration time if the timer is already set.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  audio/audio.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/audio/audio.c b/audio/audio.c
> > index c845a44..1ee95a5 100644
> > --- a/audio/audio.c
> > +++ b/audio/audio.c
> > @@ -1112,8 +1112,10 @@ static int audio_is_timer_needed (void)
> >  static void audio_reset_timer (AudioState *s)
> >  {
> >      if (audio_is_timer_needed ()) {
> > -        timer_mod (s->ts,
> > -            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
> > +        if (!timer_pending(s->ts)) {
> > +            timer_mod (s->ts,
> > +                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> conf.period.ticks);
> > +        }
> >      }
> >      else {
> >          timer_del (s->ts);
>
>
>
>


-- 
Thanks, Yurii

Re: [Qemu-devel] [PATCH] audio: make audio poll timer deterministic

[Paolo Bonzini]

On 13/02/2017 19:40, Yurii Zubrytskyi wrote:
> Hi,
> 
> It looks to me that this behavior can be achieved with
> "timer_mod_anticipate()" function instead of a separate check.

True, but I think Pavel's version is more readable.  Since the timer is
set to a fixed value from QEMU_CLOCK_VIRTUAL, timer_mod_anticipate is
never going to do anything if the timer is already set.

Paolo

>     > diff --git a/audio/audio.c b/audio/audio.c
>     > index c845a44..1ee95a5 100644
>     > --- a/audio/audio.c
>     > +++ b/audio/audio.c
>     > @@ -1112,8 +1112,10 @@ static int audio_is_timer_needed (void)
>     >  static void audio_reset_timer (AudioState *s)
>     >  {
>     >      if (audio_is_timer_needed ()) {
>     > -        timer_mod (s->ts,
>     > -            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>     conf.period.ticks);
>     > +        if (!timer_pending(s->ts)) {
>     > +            timer_mod (s->ts,
>     > +                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>     conf.period.ticks);
>     > +        }
>     >      }
>     >      else {
>     >          timer_del (s->ts);
> 
> 
> 
> 
> 
> 
> -- 
> Thanks, Yurii