Patchwork [2/8] timers: add timer_mod_anticipate and timer_mod_anticipate_ns

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 8, 2013, 8:47 a.m.
Message ID <1381222058-16701-3-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/281362/
State New
Headers show

Comments

Paolo Bonzini - Oct. 8, 2013, 8:47 a.m.
These let a user anticipate the deadline of a timer, atomically with
other sites that call the function.  This helps avoiding complicated
lock hierarchies.  It is useful whenever the timer does work based on
the current value of the clock (rather than doing something periodically
on every tick).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/timer.h | 26 ++++++++++++++++++++++++++
 qemu-timer.c         | 29 +++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)
Alex Bligh - Oct. 8, 2013, 9:15 a.m.
Paolo,

On 8 Oct 2013, at 09:47, Paolo Bonzini wrote:
> 
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -393,11 +393,40 @@ void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
>     }
> }
> 
> +/* modify the current timer so that it will be fired when current_time
> +   >= expire_time or the current deadline, whichever comes earlier.
> +   The corresponding callback will be called. */
> +void timer_mod_anticipate_ns(QEMUTimer *ts, int64_t expire_time)
> +{
> +    QEMUTimerList *timer_list = ts->timer_list;
> +    bool rearm;
> +
> +    qemu_mutex_lock(&timer_list->active_timers_lock);
> +    if (ts->expire_time == -1 || ts->expire_time > expire_time) {

So "if we want to alter it" ...

> +        if (ts->expire_time != -1) {
> +            timer_del_locked(timer_list, ts);
> +        }

What's this bit for? Surely you've calculated whether you are
shortening the expiry time (above), so all you need do now is
modify it. Why delete it? timer_mod_ns doesn't make this
check?

Otherwise looks OK.

> +        rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
> +    } else {
> +        rearm = false;
> +    }
> +    qemu_mutex_unlock(&timer_list->active_timers_lock);
> +
> +    if (rearm) {
> +        timerlist_rearm(timer_list);
> +    }
> +}
> +
> void timer_mod(QEMUTimer *ts, int64_t expire_time)
> {
>     timer_mod_ns(ts, expire_time * ts->scale);
> }
> 
> +void timer_mod_anticipate(QEMUTimer *ts, int64_t expire_time)
> +{
> +    timer_mod_anticipate_ns(ts, expire_time * ts->scale);
> +}
> +
> bool timer_pending(QEMUTimer *ts)
> {
>     return ts->expire_time >= 0;
> -- 
> 1.8.3.1
> 
> 
> 
>
Paolo Bonzini - Oct. 8, 2013, 9:25 a.m.
Il 08/10/2013 11:15, Alex Bligh ha scritto:
> So "if we want to alter it" ...
> 
>> > +        if (ts->expire_time != -1) {
>> > +            timer_del_locked(timer_list, ts);
>> > +        }
> What's this bit for? Surely you've calculated whether you are
> shortening the expiry time (above), so all you need do now is
> modify it. Why delete it? timer_mod_ns doesn't make this
> check?

timer_mod_ns_locked does not remove the timer from the list:

    qemu_mutex_lock(&timer_list->active_timers_lock);
    timer_del_locked(timer_list, ts);
    rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
    qemu_mutex_unlock(&timer_list->active_timers_lock);

This is doing the same.  The check in the "if" is not strictly
necessary, but it saves a useless visit of the list.  It could be added
to timer_mod_ns as well.

Paolo
Alex Bligh - Oct. 8, 2013, 5:01 p.m.
On 8 Oct 2013, at 10:25, Paolo Bonzini wrote:

> Il 08/10/2013 11:15, Alex Bligh ha scritto:
>> So "if we want to alter it" ...
>> 
>>>> +        if (ts->expire_time != -1) {
>>>> +            timer_del_locked(timer_list, ts);
>>>> +        }
>> What's this bit for? Surely you've calculated whether you are
>> shortening the expiry time (above), so all you need do now is
>> modify it. Why delete it? timer_mod_ns doesn't make this
>> check?
> 
> timer_mod_ns_locked does not remove the timer from the list:
> 
>    qemu_mutex_lock(&timer_list->active_timers_lock);
>    timer_del_locked(timer_list, ts);
>    rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
>    qemu_mutex_unlock(&timer_list->active_timers_lock);
> 
> This is doing the same.  The check in the "if" is not strictly
> necessary, but it saves a useless visit of the list.  It could be added
> to timer_mod_ns as well.

Quite right. I'd missed the timer_del somehow.

Patch

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index b58903b..f215b0b 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -539,6 +539,19 @@  void timer_del(QEMUTimer *ts);
 void timer_mod_ns(QEMUTimer *ts, int64_t expire_time);
 
 /**
+ * timer_mod_anticipate_ns:
+ * @ts: the timer
+ * @expire_time: the expiry time in nanoseconds
+ *
+ * Modify a timer to expire at @expire_time or the current time,
+ * whichever comes earlier.
+ *
+ * This function is thread-safe but the timer and its timer list must not be
+ * freed while this function is running.
+ */
+void timer_mod_anticipate_ns(QEMUTimer *ts, int64_t expire_time);
+
+/**
  * timer_mod:
  * @ts: the timer
  * @expire_time: the expire time in the units associated with the timer
@@ -552,6 +565,19 @@  void timer_mod_ns(QEMUTimer *ts, int64_t expire_time);
 void timer_mod(QEMUTimer *ts, int64_t expire_timer);
 
 /**
+ * timer_mod_anticipate:
+ * @ts: the timer
+ * @expire_time: the expiry time in nanoseconds
+ *
+ * Modify a timer to expire at @expire_time or the current time, whichever
+ * comes earlier, taking into account the scale associated with the timer.
+ *
+ * This function is thread-safe but the timer and its timer list must not be
+ * freed while this function is running.
+ */
+void timer_mod_anticipate(QEMUTimer *ts, int64_t expire_time);
+
+/**
  * timer_pending:
  * @ts: the timer
  *
diff --git a/qemu-timer.c b/qemu-timer.c
index 95fc6eb..202e9a2 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -393,11 +393,40 @@  void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)
     }
 }
 
+/* modify the current timer so that it will be fired when current_time
+   >= expire_time or the current deadline, whichever comes earlier.
+   The corresponding callback will be called. */
+void timer_mod_anticipate_ns(QEMUTimer *ts, int64_t expire_time)
+{
+    QEMUTimerList *timer_list = ts->timer_list;
+    bool rearm;
+
+    qemu_mutex_lock(&timer_list->active_timers_lock);
+    if (ts->expire_time == -1 || ts->expire_time > expire_time) {
+        if (ts->expire_time != -1) {
+            timer_del_locked(timer_list, ts);
+        }
+        rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
+    } else {
+        rearm = false;
+    }
+    qemu_mutex_unlock(&timer_list->active_timers_lock);
+
+    if (rearm) {
+        timerlist_rearm(timer_list);
+    }
+}
+
 void timer_mod(QEMUTimer *ts, int64_t expire_time)
 {
     timer_mod_ns(ts, expire_time * ts->scale);
 }
 
+void timer_mod_anticipate(QEMUTimer *ts, int64_t expire_time)
+{
+    timer_mod_anticipate_ns(ts, expire_time * ts->scale);
+}
+
 bool timer_pending(QEMUTimer *ts)
 {
     return ts->expire_time >= 0;