Re: [Qemu-devel] [PATCH 2/3] monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"

[Markus Armbruster]

address@hidden writes:

> From: Marc-André Lureau <address@hidden>
>
> Use a hash table to lookup the pending event corresponding to the "id"
> field. The hash table may grow without limit here, the following patch
> will add some cleaning.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  monitor.c | 104 
> ++++++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 78 insertions(+), 26 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 0a6a16a..7f5c80f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -462,10 +462,10 @@ static void monitor_qapi_event_emit(QAPIEvent event, 
> QObject *data)
>  }
>  
>  static bool
> -monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
> +monitor_qapi_event_pending_update(MonitorQAPIEventState *evstate,
> +                                  MonitorQAPIEventPending *p, QDict *data)

Function name gets even longer, yet I miss a function comment as much as
before :)

Since this is the common part of monitor_qapi_event_delay() and
monitor_qapi_event_id_delay(), what about calling it
monitor_qapi_event_do_delay()?

>  {
>      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 */
> @@ -494,31 +494,13 @@ monitor_qapi_event_delay(MonitorQAPIEventState 
> *evstate, QDict *data)
>      return false;
>  }
>  
> -/*
> - * Queue a new event for emission to Monitor instances,
> - * applying any rate limiting if required.
> - */
> -static void
> -monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
> -{
> -    MonitorQAPIEventState *evstate;
> -    assert(event < QAPI_EVENT_MAX);
> -    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -
> -    evstate = &(monitor_qapi_event_state[event]);
> -    trace_monitor_protocol_event_queue(event,
> -                                       data,
> -                                       evstate->rate,
> -                                       now);
>  
> -    qemu_mutex_lock(&monitor_lock);
> -
> -    if (!evstate->delay ||
> -        !evstate->delay(evstate, data)) {
> -        monitor_qapi_event_emit(event, QOBJECT(data));
> -    }

No change, just diff being insufficiently smart.

> +static bool
> +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
> +{
> +    MonitorQAPIEventPending *p = evstate->data;
>  
> -    qemu_mutex_unlock(&monitor_lock);
> +    return monitor_qapi_event_pending_update(evstate, p, data);
>  }

Why not simply

    return monitor_qapi_event_pending_update(evstate, evstate->data, data);

>  
>  /*
> @@ -558,6 +540,58 @@ monitor_qapi_event_pending_new(QAPIEvent event)
>      return p;
>  }
>  
> +static bool
> +monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
> +{
> +    GHashTable *ht = evstate->data;
> +    QDict *s = qdict_get_qdict(data, "data");

Why is this called s?

Here are less confusing names:

* The parameter should not be called data, because it's the complete QMP
  event object, and *not* its data member.  Either match
  QMPEventFuncEmit and call it qdict, or call it event.

* The variable s *is* the data member, and should therefore be called data.

> +    const char *id = qdict_get_str(s, "id");

This assumes s has a member "id" of type QString.  Therefore, the
function can only be used for events where that is always the case.
Unobvious requirements like that should be spelled out in a function
comment.

> +    MonitorQAPIEventPending *p = g_hash_table_lookup(ht, id);
> +    QAPIEvent event = evstate - monitor_qapi_event_state;

Another unobvious requirement: evstate must point into
monitor_qapi_event_state.

> +
> +    if (!p) {
> +        p = monitor_qapi_event_pending_new(event);
> +        g_hash_table_insert(ht, g_strdup(id), p);

Note for later: both key and value are dynamically allocated.

> +    }
> +
> +    return monitor_qapi_event_pending_update(evstate, p, data);
> +}
> +
> +/*
> + * Queue a new event for emission to Monitor instances,
> + * applying any rate limiting if required.
> + */
> +static void
> +monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
> +{
> +    MonitorQAPIEventState *evstate;
> +    assert(event < QAPI_EVENT_MAX);
> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +
> +    evstate = &(monitor_qapi_event_state[event]);
> +    trace_monitor_protocol_event_queue(event,
> +                                       data,
> +                                       evstate->rate,
> +                                       now);
> +
> +    qemu_mutex_lock(&monitor_lock);
> +
> +    if (!evstate->delay ||
> +        !evstate->delay(evstate, data)) {
> +        monitor_qapi_event_emit(event, QOBJECT(data));
> +    }
> +
> +    qemu_mutex_unlock(&monitor_lock);
> +}
> +
> +static void
> +monitor_qapi_event_pending_free(MonitorQAPIEventPending *p)
> +{
> +    timer_del(p->timer);
> +    timer_free(p->timer);
> +    g_free(p);

Ignorant question: who owns p->data?

> +}
> +
>  /*
>   * @event: the event ID to be limited
>   * @rate: the rate limit in milliseconds
> @@ -582,6 +616,24 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t 
> rate)
>      evstate->data = monitor_qapi_event_pending_new(event);
>  }
>  
> +static void
> +monitor_qapi_event_id_throttle(QAPIEvent event, int64_t rate)
> +{
> +    MonitorQAPIEventState *evstate;
> +    assert(event < QAPI_EVENT_MAX);
> +
> +    evstate = &(monitor_qapi_event_state[event]);

Superfluous parenthesis.

> +
> +    trace_monitor_protocol_event_throttle(event, rate);
> +    assert(rate * SCALE_MS <= INT64_MAX);
> +    evstate->rate = rate * SCALE_MS;
> +
> +    evstate->delay = monitor_qapi_event_id_delay;
> +    evstate->data =
> +        g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
> +            (GDestroyNotify)monitor_qapi_event_pending_free);

Both key and value are dynamically allocated.  But you define only the
value's free callback.  Why?

> +}
> +
>  static void monitor_qapi_event_init(void)
>  {
>      /* Limit guest-triggerable events to 1 per second */
> @@ -590,7 +642,7 @@ static void monitor_qapi_event_init(void)
>      monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000);
>      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
>      monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
> -    monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
> +    monitor_qapi_event_id_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
>  
>      qmp_event_set_func_emit(monitor_qapi_event_queue);
>  }

I got a few more questions.

Do we expect more events in need of separate throttling by some ID-like
member of their data object?

Do we expect *different* callbacks?  Callbacks that throttle by some
member of their data object won't count as different.

Do we really need the callback and the type punning?  As long as all we
have is the current two throttles, I'd try to do without, as follows.

monitor_qapi_event_throttle() takes the name of a data member as
additional argument.  If null, throttle like monitor_qapi_event_delay().
Else, throttle like monitor_qapi_event_id_delay().  Except do it in a
single function, with a single MonitorQAPIEventState.  The natural
representation of "either a single MonitorQAPIEventPending or a hash
table of them" is a union.