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

[Marc-André Lureau]

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.
I would still argue that it's cleaner and faster the way I proposed.
ymmv

> RFC because it's compile-tested only.  Compile-tested only because I
> want to give my feedback to Marc-André's work as soon as I can
> (although "as soon as I can" still means "frustratingly late", I'm
> afraid).
>
> PATCH 1+2 clean up the code some before I start.  PATCH 2 may fix
> bugs, but to be sure I'd have to think some more about the old code.
>
> PATCH 3-5 do the actual work.
>
> PATCH 6 fixes docs to mention throttling.
>
> Diffstat for monitor.c
>     Marc-André's series: 201 insertions(+), 55 deletions(-)
>     mine:                109 insertions(+), 77 deletions(-)
>
> Markus Armbruster (6):
>   monitor: Reduce casting of QAPI event QDict
>   monitor: Simplify event throttling
>   monitor: Split MonitorQAPIEventConf off MonitorQAPIEventState
>   monitor: Turn monitor_qapi_event_state[] into a hash table
>   monitor: Throttle event VSERPORT_CHANGE separately by "id"
>   docs: Document QMP event rate limiting
>
>  docs/qmp/qmp-events.txt |  12 ++++
>  docs/qmp/qmp-spec.txt   |   5 ++
>  monitor.c               | 186 
> ++++++++++++++++++++++++++++--------------------
>  3 files changed, 126 insertions(+), 77 deletions(-)
>
> --
> 2.4.3
>
>



-- 
Marc-André Lureau