Re: [Qemu-devel] [RFC 0/6] Throttle event VSERPORT_CHANGE separately by "id"

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Fri, Sep 25, 2015 at 4:00 PM, Markus Armbruster <address@hidden> wrote:
>> VSERPORT_CHANGE is emitted when the guest opens or closes a
>> virtio-serial port.  The event's member "id" identifies the port.
>>
>> When several events arrive quickly, throttling drops all but the last
>> of them.  Because of that, a QMP client must assume that *any* port
>> may have changed state when it receives a VSERPORT_CHANGE event and
>> throttling may have happened.
>>
>> Make the event more useful by throttling it for each port separately.
>>
>> Marc-André recently posted 'monitor: throttle VSERPORT_CHANGED by
>> "id"' to do the same thing.  Why am I redoing his work?  Let me
>> explain.
>>
>> In master, event throttling works per QAPIEvent.  Throttling state is
>> kept in MonitorQAPIEventState monitor_qapi_event_state[].  Going from
>> event to throttling state is a simple array subscript.
>>
>> Marc-André's series makes the code governing throttling pluggable per
>> event.  MonitorQAPIEventState gets two new members, the governor
>> function and an opaque.  It loses the actual throttling state.  That
>> one's now in new type MonitorQAPIEventPending.
>>
>> The standard governor has the opaque point to a single
>> MonitorQAPIEventPending.
>>
>> The governor for VSERPORT_CHANGED has the opaque point to a hash table
>> mapping "id" to MonitorQAPIEventPending.
>>
>> In my review, I challenged the complexity of this solution, but
>> Marc-André thinks it's justified.  That leaves me two choices: concede
>> the point, or come up with something that's actually simpler.
>>
>> My solution doesn't make anything pluggable.  Instead, I simply
>> generalize the map from event to throttling state.  Instead of using
>> just the QAPIEvent as key for looking up state, I permit additional
>> use of event-specific data.  For VSERPORT_CHANGED, I additionally use
>> "id".  monitor_qapi_event_state[] becomes a hash table.
>>
>> No callbacks, no type-punning, less code, and fewer code paths to
>> test.
>
> I dislike the approach to put all event filters in the same hashtable.
> The simple case doesn't need it, and I kept it that way. But
> certainly, it shorten a bit the code. Since it is probably not
> performance critical, it doesn't matter, so you may prefer less code.

Yes, I do prefer simpler code.  Fewer bugs, cheaper to maintain.

The performance differences aren't entirely obvious.  Sure, going
through a hash table is slower than a simple array subscript.  But your
indirections aren't free, either.  Mispredicted indirect calls, in
particular.

> I would still argue that it's cleaner and faster the way I proposed.
> ymmv

[...]