Re: [Qemu-devel] [RFC 2/6] monitor: Simplify event throttling

[Eric Blake]

On 09/25/2015 08:00 AM, Markus Armbruster wrote:
> The event throttling state machine is hard to understand.  I'm not
> sure it's entirely correct.  Rewrite it in a more straightforward
> manner:
> 
> State 1: No event sent recently (less than evconf->rate ns ago)
> 
>     Invariant: evstate->timer is not pending, evstate->qdict is null
> 
>     On event: send event, arm timer, goto state 2
> 
> State 2: Event sent recently, no additional event being delayed
> 
>     Invariant: evstate->timer is pending, evstate->qdict is null
> 
>     On event: store it in evstate->qdict, goto state 3
> 
>     On timer: goto state 1
> 
> State 3: Event sent recently, additional event being delayed
> 
>     Invariant: evstate->timer is pending, evstate->qdict is non-null
> 
>     On event: store it in evstate->qdict, goto state 3
> 
>     On timer: send evstate->qdict, clear evstate->qdict,
>               arm timer, goto state 2
> 

Makes sense for what throttling is supposed to be.

> FIXME update trace-event (and recompile the world)

Obviously something you'd fold into a v2, so I'll defer R-b until seeing
it.  But I like the approach of this patch.

> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  monitor.c | 70 
> +++++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
> 

>      if (!evstate->rate) {
> +        /* Unthrottled event */
>          monitor_qapi_event_emit(event, qdict);
> -        evstate->last = now;
>      } else {
> -        int64_t delta = now - evstate->last;
> -        if (evstate->qdict ||
> -            delta < evstate->rate) {
> -            /* If there's an existing event pending, replace
> -             * it with the new event, otherwise schedule a
> -             * timer for delayed emission
> +        if (timer_pending(evstate->timer)) {

Possible in states 2 and 3...

> +            /*
> +             * Timer is pending for (at least) evstate->rate ns after
> +             * last send.  Store event for sending when timer fires,
> +             * replacing a prior stored event if any.
>               */
> -            if (evstate->qdict) {
> -                QDECREF(evstate->qdict);
> -            } else {
> -                int64_t then = evstate->last + evstate->rate;
> -                timer_mod_ns(evstate->timer, then);
> -            }
> +            QDECREF(evstate->qdict);

no-op in state 2, otherwise replaces the old pending event in state 3

>              evstate->qdict = qdict;
>              QINCREF(evstate->qdict);

either way, we are now in state 3 awaiting the next event or timer.

>          } else {

we are in state 1...

> +            /*
> +             * Last send was (at least) evstate->rate ns ago.
> +             * Send immediately, and arm the timer to call
> +             * monitor_qapi_event_handler() in evstate->rate ns.  Any
> +             * events arriving before then will be delayed until then.
> +             */
> +            int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +
>              monitor_qapi_event_emit(event, qdict);
> -            evstate->last = now;
> +            timer_mod_ns(evstate->timer, now + evstate->rate);

and now in state 2.

>          }
>      }
> +
>      qemu_mutex_unlock(&monitor_lock);
>  }
>  
>  /*
> - * The callback invoked by QemuTimer when a delayed
> - * event is ready to be emitted
> + * This function runs evstate->rate ns after sending a throttled
> + * event.
> + * If another event has since been stored, send it.
>   */
>  static void monitor_qapi_event_handler(void *opaque)
>  {

We get here in either state 2 or 3...

>      MonitorQAPIEventState *evstate = opaque;
> -    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>  
>      trace_monitor_protocol_event_handler(evstate->event,
>                                           evstate->qdict,
> -                                         evstate->last,
> -                                         now);
> +                                         0, 0); /* FIXME drop */
>      qemu_mutex_lock(&monitor_lock);
> +
>      if (evstate->qdict) {

state 3, so send it...

> +        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +
>          monitor_qapi_event_emit(evstate->event, evstate->qdict);
>          QDECREF(evstate->qdict);
>          evstate->qdict = NULL;
> +        timer_mod_ns(evstate->timer, now + evstate->rate);

...and rearm to go back to state 2

>      }
> -    evstate->last = now;
> +

...otherwise, we didn't rearm, so we go back to state 1

>      qemu_mutex_unlock(&monitor_lock);
>  }
>  

Matches the state logic called out in the commit message.

> @@ -539,20 +542,17 @@ static void
>  monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>  {
>      MonitorQAPIEventState *evstate;
> +
>      assert(event < QAPI_EVENT_MAX);
> -
> -    evstate = &(monitor_qapi_event_state[event]);
> -
> +    evstate = &monitor_qapi_event_state[event];
>      trace_monitor_protocol_event_throttle(event, rate);
>      evstate->event = event;
>      assert(rate * SCALE_MS <= INT64_MAX);
>      evstate->rate = rate * SCALE_MS;
> -    evstate->last = 0;
>      evstate->qdict = NULL;
> -    evstate->timer = timer_new(QEMU_CLOCK_REALTIME,
> -                               SCALE_MS,
> -                               monitor_qapi_event_handler,
> -                               evstate);
> +    evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
> +                                  monitor_qapi_event_handler,
> +                                  evstate);

Is this a separate cleanup?  It looks like you're fixing code style, and
then switching from timer_new() to timer_new_ns(), but it's not obvious
from just this context whether you still have the right scale (and I
didn't go chase documentation).  It wasn't mentioned in the commit
message, and seems unrelated to the new state machine.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature