Hi On Fri, Sep 25, 2015 at 4:00 PM, Markus Armbruster <arm...@redhat.com> 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
