| Message ID | 20180608035511.7439-6-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | monitor: let Monitor be thread safe | expand |
Peter Xu <peterx@redhat.com> writes: > Instead, use a dynamic function to detect which clock we'll use. The > problem is that the old code will let monitor initialization depend on > configure_accelerator() (that's where qtest_enabled() start to take > effect). After this change, we don't have such a dependency any more. > We just need to make sure configure_accelerator() is called when we > start to use it. Now it's only used in monitor_qapi_event_queue() and > monitor_qapi_event_handler(), so we're good. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > monitor.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/monitor.c b/monitor.c > index fa9c35631f..2bda0dfa12 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -281,8 +281,6 @@ QmpCommandList qmp_commands, qmp_cap_negotiation_commands; > > Monitor *cur_mon; > > -static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME; > - > static void monitor_command_cb(void *opaque, const char *cmdline, > void *readline_opaque); > > @@ -309,6 +307,19 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon) > return !monitor_is_qmp(mon) && !monitor_uses_readline(mon); > } > > +/* > + * This should never be called before configure_accelerator() since > + * only until then could we know whether qtest was enabled or not. Uh, we know it after then, not until then. What about /* * Return the clock to use for recording an event's time. * Beware: result is invalid before configure_accelerator(). > + */ > +static inline QEMUClockType monitor_get_clock(void) Suggest to rename to monitor_get_event_clock(), or just get_event_clock(). Happy to apply touch-ups on commit. > +{ > + /* > + * This allows us to perform tests on the monitor queues to verify > + * that the rate limits are enforced. > + */ > + return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME; > +} > + > /** > * Is the current monitor, if any, a QMP monitor? > */ > @@ -632,7 +643,7 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp) > * monitor_qapi_event_handler() in evconf->rate ns. Any > * events arriving before then will be delayed until then. > */ > - int64_t now = qemu_clock_get_ns(event_clock_type); > + int64_t now = qemu_clock_get_ns(monitor_get_clock()); > > monitor_qapi_event_emit(event, qdict); > > @@ -640,7 +651,7 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp) > evstate->event = event; > evstate->data = qobject_ref(data); > evstate->qdict = NULL; > - evstate->timer = timer_new_ns(event_clock_type, > + evstate->timer = timer_new_ns(monitor_get_clock(), > monitor_qapi_event_handler, > evstate); > g_hash_table_add(monitor_qapi_event_state, evstate); > @@ -665,7 +676,7 @@ static void monitor_qapi_event_handler(void *opaque) > qemu_mutex_lock(&monitor_lock); > > if (evstate->qdict) { > - int64_t now = qemu_clock_get_ns(event_clock_type); > + int64_t now = qemu_clock_get_ns(monitor_get_clock()); > > monitor_qapi_event_emit(evstate->event, evstate->qdict); > qobject_unref(evstate->qdict); > @@ -721,10 +732,6 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b) > > static void monitor_qapi_event_init(void) > { > - if (qtest_enabled()) { > - event_clock_type = QEMU_CLOCK_VIRTUAL; > - } > - > monitor_qapi_event_state = g_hash_table_new(qapi_event_throttle_hash, > qapi_event_throttle_equal); > qmp_event_set_func_emit(monitor_qapi_event_queue); With at least a suitable fix for the comment: Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Fri, Jun 08, 2018 at 07:38:11AM +0200, Markus Armbruster wrote: [...] > > +/* > > + * This should never be called before configure_accelerator() since > > + * only until then could we know whether qtest was enabled or not. > > Uh, we know it after then, not until then. What about > > /* > * Return the clock to use for recording an event's time. > * Beware: result is invalid before configure_accelerator(). Oh, another Chinglish from me. :( > > > + */ > > +static inline QEMUClockType monitor_get_clock(void) > > Suggest to rename to monitor_get_event_clock(), or just > get_event_clock(). > > Happy to apply touch-ups on commit. That'll be appreciated. Both changes work for me. [...] > With at least a suitable fix for the comment: > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks!
Peter Xu <peterx@redhat.com> writes: > On Fri, Jun 08, 2018 at 07:38:11AM +0200, Markus Armbruster wrote: > > [...] > >> > +/* >> > + * This should never be called before configure_accelerator() since >> > + * only until then could we know whether qtest was enabled or not. >> >> Uh, we know it after then, not until then. What about >> >> /* >> * Return the clock to use for recording an event's time. >> * Beware: result is invalid before configure_accelerator(). > > Oh, another Chinglish from me. :( Useful code, occasional Chinglish, the humility to go with it, and a sense of humor --- you're doing fine. [...]
On Fri, Jun 08, 2018 at 11:55:09AM +0800, Peter Xu wrote: > Instead, use a dynamic function to detect which clock we'll use. The > problem is that the old code will let monitor initialization depend on > configure_accelerator() (that's where qtest_enabled() start to take > effect). After this change, we don't have such a dependency any more. > We just need to make sure configure_accelerator() is called when we > start to use it. Now it's only used in monitor_qapi_event_queue() and > monitor_qapi_event_handler(), so we're good. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > monitor.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) With Markus' suggestion: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Fri, Jun 08, 2018 at 09:09:57AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Fri, Jun 08, 2018 at 07:38:11AM +0200, Markus Armbruster wrote: > > > > [...] > > > >> > +/* > >> > + * This should never be called before configure_accelerator() since > >> > + * only until then could we know whether qtest was enabled or not. > >> > >> Uh, we know it after then, not until then. What about > >> > >> /* > >> * Return the clock to use for recording an event's time. > >> * Beware: result is invalid before configure_accelerator(). > > > > Oh, another Chinglish from me. :( > > Useful code, occasional Chinglish, the humility to go with it, and a > sense of humor --- you're doing fine. Very high evaluation. Thank you. :)
diff --git a/monitor.c b/monitor.c index fa9c35631f..2bda0dfa12 100644 --- a/monitor.c +++ b/monitor.c @@ -281,8 +281,6 @@ QmpCommandList qmp_commands, qmp_cap_negotiation_commands; Monitor *cur_mon; -static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME; - static void monitor_command_cb(void *opaque, const char *cmdline, void *readline_opaque); @@ -309,6 +307,19 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon) return !monitor_is_qmp(mon) && !monitor_uses_readline(mon); } +/* + * This should never be called before configure_accelerator() since + * only until then could we know whether qtest was enabled or not. + */ +static inline QEMUClockType monitor_get_clock(void) +{ + /* + * This allows us to perform tests on the monitor queues to verify + * that the rate limits are enforced. + */ + return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME; +} + /** * Is the current monitor, if any, a QMP monitor? */ @@ -632,7 +643,7 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp) * monitor_qapi_event_handler() in evconf->rate ns. Any * events arriving before then will be delayed until then. */ - int64_t now = qemu_clock_get_ns(event_clock_type); + int64_t now = qemu_clock_get_ns(monitor_get_clock()); monitor_qapi_event_emit(event, qdict); @@ -640,7 +651,7 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp) evstate->event = event; evstate->data = qobject_ref(data); evstate->qdict = NULL; - evstate->timer = timer_new_ns(event_clock_type, + evstate->timer = timer_new_ns(monitor_get_clock(), monitor_qapi_event_handler, evstate); g_hash_table_add(monitor_qapi_event_state, evstate); @@ -665,7 +676,7 @@ static void monitor_qapi_event_handler(void *opaque) qemu_mutex_lock(&monitor_lock); if (evstate->qdict) { - int64_t now = qemu_clock_get_ns(event_clock_type); + int64_t now = qemu_clock_get_ns(monitor_get_clock()); monitor_qapi_event_emit(evstate->event, evstate->qdict); qobject_unref(evstate->qdict); @@ -721,10 +732,6 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b) static void monitor_qapi_event_init(void) { - if (qtest_enabled()) { - event_clock_type = QEMU_CLOCK_VIRTUAL; - } - monitor_qapi_event_state = g_hash_table_new(qapi_event_throttle_hash, qapi_event_throttle_equal); qmp_event_set_func_emit(monitor_qapi_event_queue);
Instead, use a dynamic function to detect which clock we'll use. The problem is that the old code will let monitor initialization depend on configure_accelerator() (that's where qtest_enabled() start to take effect). After this change, we don't have such a dependency any more. We just need to make sure configure_accelerator() is called when we start to use it. Now it's only used in monitor_qapi_event_queue() and monitor_qapi_event_handler(), so we're good. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- monitor.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)