Re: [Qemu-devel] [RFC 1/3] monitor: split MonitorQAPIEventState

[Eric Blake]

On 08/12/2015 01:46 PM, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> Create a seperate pending event structure MonitorQAPIEventPending.

s/seperate/separate/

> Use a MonitorQAPIEventDelay callback to handle the delaying. This
> allows other implementations of throttling.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  monitor.c| 124 
> +--
>  trace-events |   2 +-
>  2 files changed, 79 insertions(+), 47 deletions(-)
> 

>  
> +typedef struct MonitorQAPIEventPending {
> +QAPIEvent event;/* Event being tracked */
> +int64_t last;   /* QEMU_CLOCK_REALTIME value at last emission */
> +QEMUTimer *timer;   /* Timer for handling delayed events */
> +QObject *data;  /* Event pending delayed dispatch */
> +} MonitorQAPIEventPending;
> +
> +typedef struct MonitorQAPIEventState MonitorQAPIEventState;

Some places combine the typedef with the struct definition; I'm not sure
there's any hard and fast rule, though. (HACKING mentions that we want
the typedef, but doesn't give guidelines on how it must be provided).

> +struct MonitorQAPIEventState {
>  int64_t rate;   /* Minimum time (in ns) between two events */
> -int64_t last;   /* QEMU_CLOCK_REALTIME value at last emission */
> -QEMUTimer *timer;   /* Timer for handling delayed events */
> -QObject *data;  /* Event pending delayed dispatch */
> -} MonitorQAPIEventState;
> +MonitorQAPIEventDelay delay;
> +gpointer data;

Do we really need 'gpointer', or is 'void *' sufficient?

>  
> +static bool
> +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
> +{
> +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 */
> +if (!evstate->rate) {
> +p->last = now;
> +return FALSE;

s/FALSE/false/ (we want to directly use the C99 'bool' type here, not
the glib macros that expand to who-knows-what-type followed by implicit
conversion back to bool).

> +p->data = QOBJECT(data);
> +qobject_incref(p->data);
> +return TRUE;
> +}
> +
> +p->last = now;
> +return FALSE;

two more ALL_CAPS to convert to the lower bool counterpart.

Otherwise looks like a sane split.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC 1/3] monitor: split MonitorQAPIEventState

[Eric Blake]

On 08/12/2015 02:00 PM, Laszlo Ersek wrote:
 Assume there has been a long period of silence (no attempts to emit an
 event). Now some client code makes a call to emit the event.
 
 Will that event be emitted immediately, or will it be delayed to see if
 more are coming? I'd like to understand this aspect first.
 
 I think the first instance of the event, after the grace period, should
 be emitted immediately, and further instances that quickly follow should
 be suppressed.

That has always been the goal of event throttling: when a new event
arrives and the timer is not running, emit it right away and start the
timer. If another event arrives while the timer has not expired, save
it.  If multiple events arrive during the timer, the last one received
overwrites any others.  Then, when the timer expires, either there is a
saved event (one or more events arrived during the throttling window),
so we send it and restart another throttling window, or there was no
event pending so the line is quiet and the next event can send right
away.  Thus, management will get notification as soon as possible that
events are starting to happen after a period of quiet, but will then not
be spammed any faster than once per throttle period (once per second)
with additional events.  When additional events are sent, they are
always the most accurate state of the system at the time the event was
queued, but intermediate events may have been lost.  Furthermore, unless
the events have not been happening, the throttling of the event means
the management might not see the event until nearly a second late,
although the timestamp associated with the event should still be
accurate to the time it was queued up, not finally sent.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [RFC 1/3] monitor: split MonitorQAPIEventState

[Marc-André Lureau]

Hi

- Original Message -
 On 08/12/15 21:46, marcandre.lur...@redhat.com wrote:
  From: Marc-André Lureau marcandre.lur...@redhat.com
  
  Create a seperate pending event structure MonitorQAPIEventPending.
  Use a MonitorQAPIEventDelay callback to handle the delaying. This
  allows other implementations of throttling.
  
  Signed-off-by: Marc-André Lureau marcandre.lur...@redhat.com
  ---
   monitor.c| 124
   +--
   trace-events |   2 +-
   2 files changed, 79 insertions(+), 47 deletions(-)
 
 Assume there has been a long period of silence (no attempts to emit an
 event). Now some client code makes a call to emit the event.
 
 Will that event be emitted immediately, or will it be delayed to see if
 more are coming? I'd like to understand this aspect first.
 
 I think the first instance of the event, after the grace period, should
 be emitted immediately, and further instances that quickly follow should
 be suppressed.

This is what qemu already does. The first event is sent immediately, the
later ones may be delayed (but there will be at least one event every period,
if the event is flooded). This patch 1/3 doesn't change the logic, only
it split things to make them a bit more modular.

So the rest of the patches do not change the qemu delay logic, it adds a way to
delay based on (event + id) instead of just (event). It does that by
adding an additional id hashtable for the event type. My hope is
that this apporach could be reused if other field or combinations of fields
are necessary for other events, but for now, it's hardcoded for id. 

cheers

[Qemu-devel] [RFC 1/3] monitor: split MonitorQAPIEventState

[marcandre . lureau]

From: Marc-André Lureau marcandre.lur...@redhat.com

Create a seperate pending event structure MonitorQAPIEventPending.
Use a MonitorQAPIEventDelay callback to handle the delaying. This
allows other implementations of throttling.

Signed-off-by: Marc-André Lureau marcandre.lur...@redhat.com
---
 monitor.c| 124 +--
 trace-events |   2 +-
 2 files changed, 79 insertions(+), 47 deletions(-)

diff --git a/monitor.c b/monitor.c
index aeea2b5..9c51ffa 100644
--- a/monitor.c
+++ b/monitor.c
@@ -170,18 +170,27 @@ typedef struct {
 bool in_command_mode;   /* are we in command mode? */
 } MonitorQMP;
 
+typedef struct MonitorQAPIEventPending {
+QAPIEvent event;/* Event being tracked */
+int64_t last;   /* QEMU_CLOCK_REALTIME value at last emission */
+QEMUTimer *timer;   /* Timer for handling delayed events */
+QObject *data;  /* Event pending delayed dispatch */
+} MonitorQAPIEventPending;
+
+typedef struct MonitorQAPIEventState MonitorQAPIEventState;
+
+typedef bool (*MonitorQAPIEventDelay) (MonitorQAPIEventState *evstate,
+   QDict *data);
 /*
  * To prevent flooding clients, events can be throttled. The
  * throttling is calculated globally, rather than per-Monitor
  * instance.
  */
-typedef struct MonitorQAPIEventState {
-QAPIEvent event;/* Event being tracked */
+struct MonitorQAPIEventState {
 int64_t rate;   /* Minimum time (in ns) between two events */
-int64_t last;   /* QEMU_CLOCK_REALTIME value at last emission */
-QEMUTimer *timer;   /* Timer for handling delayed events */
-QObject *data;  /* Event pending delayed dispatch */
-} MonitorQAPIEventState;
+MonitorQAPIEventDelay delay;
+gpointer data;
+};
 
 struct Monitor {
 CharDriverState *chr;
@@ -452,6 +461,39 @@ static void monitor_qapi_event_emit(QAPIEvent event, 
QObject *data)
 }
 }
 
+static bool
+monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
+{
+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 */
+if (!evstate-rate) {
+p-last = now;
+return FALSE;
+}
+
+if (p-data || delta  evstate-rate) {
+/* If there's an existing event pending, replace
+ * it with the new event, otherwise schedule a
+ * timer for delayed emission
+ */
+if (p-data) {
+qobject_decref(p-data);
+} else {
+int64_t then = p-last + evstate-rate;
+timer_mod_ns(p-timer, then);
+}
+p-data = QOBJECT(data);
+qobject_incref(p-data);
+return TRUE;
+}
+
+p-last = now;
+return FALSE;
+}
+
 /*
  * Queue a new event for emission to Monitor instances,
  * applying any rate limiting if required.
@@ -467,35 +509,15 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, 
Error **errp)
 trace_monitor_protocol_event_queue(event,
data,
evstate-rate,
-   evstate-last,
now);
 
-/* Rate limit of 0 indicates no throttling */
 qemu_mutex_lock(monitor_lock);
-if (!evstate-rate) {
+
+if (!evstate-delay ||
+!evstate-delay(evstate, data)) {
 monitor_qapi_event_emit(event, QOBJECT(data));
-evstate-last = now;
-} else {
-int64_t delta = now - evstate-last;
-if (evstate-data ||
-delta  evstate-rate) {
-/* If there's an existing event pending, replace
- * it with the new event, otherwise schedule a
- * timer for delayed emission
- */
-if (evstate-data) {
-qobject_decref(evstate-data);
-} else {
-int64_t then = evstate-last + evstate-rate;
-timer_mod_ns(evstate-timer, then);
-}
-evstate-data = QOBJECT(data);
-qobject_incref(evstate-data);
-} else {
-monitor_qapi_event_emit(event, QOBJECT(data));
-evstate-last = now;
-}
 }
+
 qemu_mutex_unlock(monitor_lock);
 }
 
@@ -505,23 +527,37 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, 
Error **errp)
  */
 static void monitor_qapi_event_handler(void *opaque)
 {
-MonitorQAPIEventState *evstate = opaque;
+MonitorQAPIEventPending *p = opaque;
 int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
-trace_monitor_protocol_event_handler(evstate-event,
- evstate-data,
- evstate-last,
+trace_monitor_protocol_event_handler(p-event,
+ p-data,
+ p-last,
   

Re: [Qemu-devel] [RFC 1/3] monitor: split MonitorQAPIEventState

[Laszlo Ersek]

On 08/12/15 21:46, marcandre.lur...@redhat.com wrote:
 From: Marc-André Lureau marcandre.lur...@redhat.com
 
 Create a seperate pending event structure MonitorQAPIEventPending.
 Use a MonitorQAPIEventDelay callback to handle the delaying. This
 allows other implementations of throttling.
 
 Signed-off-by: Marc-André Lureau marcandre.lur...@redhat.com
 ---
  monitor.c| 124 
 +--
  trace-events |   2 +-
  2 files changed, 79 insertions(+), 47 deletions(-)

Assume there has been a long period of silence (no attempts to emit an
event). Now some client code makes a call to emit the event.

Will that event be emitted immediately, or will it be delayed to see if
more are coming? I'd like to understand this aspect first.

I think the first instance of the event, after the grace period, should
be emitted immediately, and further instances that quickly follow should
be suppressed.

It would also be possible to delay (and update) a current instance
in-place until the calls quiesce, and emit the (most recent) update
then. (I'm getting the impression that the patch does this, but I could
be wrong.)

I think emitting the first immediately (and suppressing followers) is
better; that's eg. what the kernel log does.

Nagle's algorithm (TCP_CORK - TCP_NODELAY) is a (strained) example for
the other behavior (ie. coalesce leaders). I think that approach would
not be a natural fit for QEMU's events.

Of course, if your code already implements what I think would be more
applicable, then I shouldn't have spoken! :) Can you clarify it though
please?

Thanks!
Laszlo


 diff --git a/monitor.c b/monitor.c
 index aeea2b5..9c51ffa 100644
 --- a/monitor.c
 +++ b/monitor.c
 @@ -170,18 +170,27 @@ typedef struct {
  bool in_command_mode;   /* are we in command mode? */
  } MonitorQMP;
  
 +typedef struct MonitorQAPIEventPending {
 +QAPIEvent event;/* Event being tracked */
 +int64_t last;   /* QEMU_CLOCK_REALTIME value at last emission */
 +QEMUTimer *timer;   /* Timer for handling delayed events */
 +QObject *data;  /* Event pending delayed dispatch */
 +} MonitorQAPIEventPending;
 +
 +typedef struct MonitorQAPIEventState MonitorQAPIEventState;
 +
 +typedef bool (*MonitorQAPIEventDelay) (MonitorQAPIEventState *evstate,
 +   QDict *data);
  /*
   * To prevent flooding clients, events can be throttled. The
   * throttling is calculated globally, rather than per-Monitor
   * instance.
   */
 -typedef struct MonitorQAPIEventState {
 -QAPIEvent event;/* Event being tracked */
 +struct MonitorQAPIEventState {
  int64_t rate;   /* Minimum time (in ns) between two events */
 -int64_t last;   /* QEMU_CLOCK_REALTIME value at last emission */
 -QEMUTimer *timer;   /* Timer for handling delayed events */
 -QObject *data;  /* Event pending delayed dispatch */
 -} MonitorQAPIEventState;
 +MonitorQAPIEventDelay delay;
 +gpointer data;
 +};
  
  struct Monitor {
  CharDriverState *chr;
 @@ -452,6 +461,39 @@ static void monitor_qapi_event_emit(QAPIEvent event, 
 QObject *data)
  }
  }
  
 +static bool
 +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
 +{
 +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 */
 +if (!evstate-rate) {
 +p-last = now;
 +return FALSE;
 +}
 +
 +if (p-data || delta  evstate-rate) {
 +/* If there's an existing event pending, replace
 + * it with the new event, otherwise schedule a
 + * timer for delayed emission
 + */
 +if (p-data) {
 +qobject_decref(p-data);
 +} else {
 +int64_t then = p-last + evstate-rate;
 +timer_mod_ns(p-timer, then);
 +}
 +p-data = QOBJECT(data);
 +qobject_incref(p-data);
 +return TRUE;
 +}
 +
 +p-last = now;
 +return FALSE;
 +}
 +
  /*
   * Queue a new event for emission to Monitor instances,
   * applying any rate limiting if required.
 @@ -467,35 +509,15 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, 
 Error **errp)
  trace_monitor_protocol_event_queue(event,
 data,
 evstate-rate,
 -   evstate-last,
 now);
  
 -/* Rate limit of 0 indicates no throttling */
  qemu_mutex_lock(monitor_lock);
 -if (!evstate-rate) {
 +
 +if (!evstate-delay ||
 +!evstate-delay(evstate, data)) {
  monitor_qapi_event_emit(event, QOBJECT(data));
 -evstate-last = now;
 -} else {
 -int64_t delta = now - evstate-last;
 -if (evstate-data ||
 -delta  evstate-rate) {
 -/* If there's an