Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com>
>> ---
>> 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!
