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

[Marc-André Lureau]

Hi

On Wed, Sep 16, 2015 at 6:40 PM, Markus Armbruster <address@hidden> wrote:
> 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 :)

It's longer simply because the struct name is longer.
The function "update" MonitorQAPIEventPending.

>
> 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()?
>

That's confusing, not only it doesn't follow the struct prefix, but
it's also very close to monitor_qapi_event_delay() (the simple
throttle/delay handler)

>>  {
>>      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);

ok

>
>>
>>  /*
>> @@ -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.
>

It was called "data" before, I think because "event" is for QAPIEvent
in general, and the "data" of QDict is the actual event data.

So I guess qdict is a better name, although it's also very generic to me.

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

ok

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

It spelled in the function name, but I added a comment on top.

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

It's an element in the array. I'll add a comment and an assert.

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

value has a destroy handler being called when removing the value

The key should also be destroyed, let's fix that leak. thanks

>
>> +    }
>> +
>> +    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?
>

MonitorQAPIEventPending, but the event is freed when emitted. Let's
add a cleanup here too.

>> +}
>> +
>>  /*
>>   * @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.
>

was there before, copy-pasta. removing

>> +
>> +    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?

At first I thought the key would remain in the hashtable, it's a leftover. Fixed

>
>> +}
>> +
>>  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);
>>  }
>



-- 
Marc-André Lureau