Re: [Qemu-devel] [RFC 1/3] monitor: split MonitorQAPIEventState

[Laszlo Ersek <lersek@redhat.com>]

On 08/12/15 21:46, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Create a seperate pending event structure MonitorQAPIEventPending.
> Use a MonitorQAPIEventDelay callback to handle the delaying. This
> allows other implementations of throttling.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c    | 124 +++++++++++++++++++++++++++++++++++++----------------------
>  trace-events |   2 +-
>  2 files changed, 79 insertions(+), 47 deletions(-)

Assume there has been a long period of silence (no attempts to emit an
event). Now some client code makes a call to emit the event.

Will that event be emitted immediately, or will it be delayed to see if
more are coming? I'd like to understand this aspect first.

I think the first instance of the event, after the grace period, should
be emitted immediately, and further instances that quickly follow should
be suppressed.

It would also be possible to delay (and update) a "current instance"
in-place until the calls quiesce, and emit the (most recent) update
then. (I'm getting the impression that the patch does this, but I could
be wrong.)

I think emitting the first immediately (and suppressing followers) is
better; that's eg. what the kernel log does.

Nagle's algorithm (TCP_CORK <-> TCP_NODELAY) is a (strained) example for
the other behavior (ie. coalesce leaders). I think that approach would
not be a natural fit for QEMU's events.

Of course, if your code already implements what I think would be more
applicable, then I shouldn't have spoken! :) Can you clarify it though
please?

Thanks!
Laszlo


> diff --git a/monitor.c b/monitor.c
> index aeea2b5..9c51ffa 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -170,18 +170,27 @@ typedef struct {
>      bool in_command_mode;       /* are we in command mode? */
>  } MonitorQMP;
>  
> +typedef struct MonitorQAPIEventPending {
> +    QAPIEvent event;    /* Event being tracked */
> +    int64_t last;       /* QEMU_CLOCK_REALTIME value at last emission */
> +    QEMUTimer *timer;   /* Timer for handling delayed events */
> +    QObject *data;      /* Event pending delayed dispatch */
> +} MonitorQAPIEventPending;
> +
> +typedef struct MonitorQAPIEventState MonitorQAPIEventState;
> +
> +typedef bool (*MonitorQAPIEventDelay) (MonitorQAPIEventState *evstate,
> +                                       QDict *data);
>  /*
>   * To prevent flooding clients, events can be throttled. The
>   * throttling is calculated globally, rather than per-Monitor
>   * instance.
>   */
> -typedef struct MonitorQAPIEventState {
> -    QAPIEvent event;    /* Event being tracked */
> +struct MonitorQAPIEventState {
>      int64_t rate;       /* Minimum time (in ns) between two events */
> -    int64_t last;       /* QEMU_CLOCK_REALTIME value at last emission */
> -    QEMUTimer *timer;   /* Timer for handling delayed events */
> -    QObject *data;      /* Event pending delayed dispatch */
> -} MonitorQAPIEventState;
> +    MonitorQAPIEventDelay delay;
> +    gpointer data;
> +};
>  
>  struct Monitor {
>      CharDriverState *chr;
> @@ -452,6 +461,39 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
>      }
>  }
>  
> +static bool
> +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
> +{
> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +    MonitorQAPIEventPending *p = evstate->data;
> +    int64_t delta = now - p->last;
> +
> +    /* Rate limit of 0 indicates no throttling */
> +    if (!evstate->rate) {
> +        p->last = now;
> +        return FALSE;
> +    }
> +
> +    if (p->data || delta < evstate->rate) {
> +        /* If there's an existing event pending, replace
> +         * it with the new event, otherwise schedule a
> +         * timer for delayed emission
> +         */
> +        if (p->data) {
> +            qobject_decref(p->data);
> +        } else {
> +            int64_t then = p->last + evstate->rate;
> +            timer_mod_ns(p->timer, then);
> +        }
> +        p->data = QOBJECT(data);
> +        qobject_incref(p->data);
> +        return TRUE;
> +    }
> +
> +    p->last = now;
> +    return FALSE;
> +}
> +
>  /*
>   * Queue a new event for emission to Monitor instances,
>   * applying any rate limiting if required.
> @@ -467,35 +509,15 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
>      trace_monitor_protocol_event_queue(event,
>                                         data,
>                                         evstate->rate,
> -                                       evstate->last,
>                                         now);
>  
> -    /* Rate limit of 0 indicates no throttling */
>      qemu_mutex_lock(&monitor_lock);
> -    if (!evstate->rate) {
> +
> +    if (!evstate->delay ||
> +        !evstate->delay(evstate, data)) {
>          monitor_qapi_event_emit(event, QOBJECT(data));
> -        evstate->last = now;
> -    } else {
> -        int64_t delta = now - evstate->last;
> -        if (evstate->data ||
> -            delta < evstate->rate) {
> -            /* If there's an existing event pending, replace
> -             * it with the new event, otherwise schedule a
> -             * timer for delayed emission
> -             */
> -            if (evstate->data) {
> -                qobject_decref(evstate->data);
> -            } else {
> -                int64_t then = evstate->last + evstate->rate;
> -                timer_mod_ns(evstate->timer, then);
> -            }
> -            evstate->data = QOBJECT(data);
> -            qobject_incref(evstate->data);
> -        } else {
> -            monitor_qapi_event_emit(event, QOBJECT(data));
> -            evstate->last = now;
> -        }
>      }
> +
>      qemu_mutex_unlock(&monitor_lock);
>  }
>  
> @@ -505,23 +527,37 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
>   */
>  static void monitor_qapi_event_handler(void *opaque)
>  {
> -    MonitorQAPIEventState *evstate = opaque;
> +    MonitorQAPIEventPending *p = opaque;
>      int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>  
> -    trace_monitor_protocol_event_handler(evstate->event,
> -                                         evstate->data,
> -                                         evstate->last,
> +    trace_monitor_protocol_event_handler(p->event,
> +                                         p->data,
> +                                         p->last,
>                                           now);
>      qemu_mutex_lock(&monitor_lock);
> -    if (evstate->data) {
> -        monitor_qapi_event_emit(evstate->event, evstate->data);
> -        qobject_decref(evstate->data);
> -        evstate->data = NULL;
> +    if (p->data) {
> +        monitor_qapi_event_emit(p->event, p->data);
> +        qobject_decref(p->data);
> +        p->data = NULL;
>      }
> -    evstate->last = now;
> +    p->last = now;
>      qemu_mutex_unlock(&monitor_lock);
>  }
>  
> +static MonitorQAPIEventPending *
> +monitor_qapi_event_pending_new(QAPIEvent event)
> +{
> +    MonitorQAPIEventPending *p;
> +
> +    p = g_new0(MonitorQAPIEventPending, 1);
> +    p->event = event;
> +    p->timer = timer_new(QEMU_CLOCK_REALTIME,
> +                         SCALE_MS,
> +                         monitor_qapi_event_handler,
> +                         p);
> +    return p;
> +}
> +
>  /*
>   * @event: the event ID to be limited
>   * @rate: the rate limit in milliseconds
> @@ -539,15 +575,11 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>      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->data = NULL;
> -    evstate->timer = timer_new(QEMU_CLOCK_REALTIME,
> -                               SCALE_MS,
> -                               monitor_qapi_event_handler,
> -                               evstate);
> +
> +    evstate->delay = monitor_qapi_event_delay;
> +    evstate->data = monitor_qapi_event_pending_new(event);
>  }
>  
>  static void monitor_qapi_event_init(void)
> diff --git a/trace-events b/trace-events
> index 94bf3bb..e39ff33 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1027,7 +1027,7 @@ handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
>  monitor_protocol_emitter(void *mon) "mon %p"
>  monitor_protocol_event_handler(uint32_t event, void *data, uint64_t last, uint64_t now) "event=%d data=%p last=%" PRId64 " now=%" PRId64
>  monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
> -monitor_protocol_event_queue(uint32_t event, void *data, uint64_t rate, uint64_t last, uint64_t now) "event=%d data=%p rate=%" PRId64 " last=%" PRId64 " now=%" PRId64
> +monitor_protocol_event_queue(uint32_t event, void *data, uint64_t rate, uint64_t now) "event=%d data=%p rate=%" PRId64 " now=%" PRId64
>  monitor_protocol_event_throttle(uint32_t event, uint64_t rate) "event=%d rate=%" PRId64
>  
>  # hw/net/opencores_eth.c
>