qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [RFC 2/6] monitor: Simplify event throttling
Date: Mon, 28 Sep 2015 10:14:32 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> writes:

> 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.

I'll split it off and explain it in the commit message.

Thanks!



reply via email to

[Prev in Thread] Current Thread [Next in Thread]