[Qemu-devel] [PATCH v9 0/7] monitor: let Monitor be thread safe

[Qemu-devel] [PATCH v9 0/7] monitor: let Monitor be thread safe

[Peter Xu]

v9:
- two more patches to implement Markus's idea to init monitor earlier
  (which are patch 5 & 6)
- touch up patch 7 to init the fdset lock in monitor_init_globals()

v8:
- some wording changes according to previous comments [Markus]
- return -ENOENT too in stubs/fdset.c:monitor_fdset_get_fd() [Stefan]
- refactor the fdset functions a bit, drop "ret" where proper [Markus]
- one more patch to fix monitor_lock comment [Markus]
- regular rebase and torturing

Stefan reported this problem that in the future we might start to have
more threads operating on the same Monitor object.  This seris try to
add fundamental support for it.

Please review.  Thanks,

Peter Xu (7):
  monitor: rename out_lock to mon_lock
  monitor: protect mon->fds with mon_lock
  monitor: more comments on lock-free elements
  monitor: fix comment for monitor_lock
  monitor: remove event_clock_type
  monitor: move init global earlier
  monitor: add lock to protect mon_fdsets

 monitor.c     | 164 ++++++++++++++++++++++++++++++++++----------------
 stubs/fdset.c |   2 +-
 util/osdep.c  |   3 +-
 vl.c          |   7 +--
 4 files changed, 117 insertions(+), 59 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v9 1/7] monitor: rename out_lock to mon_lock

[Peter Xu]

The out_lock is protecting a few Monitor fields.  In the future the
monitor code will start to run in multiple threads.  We are going to
turn it into a bigger lock to protect not only the out buffer but also
most of the rest.

Since at it, rearrange the Monitor struct a bit.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 53 +++++++++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/monitor.c b/monitor.c
index 46814af533..14c681dc8a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -207,15 +207,6 @@ struct Monitor {
     int suspend_cnt;            /* Needs to be accessed atomically */
     bool skip_flush;
     bool use_io_thr;
-
-    /* We can't access guest memory when holding the lock */
-    QemuMutex out_lock;
-    QString *outbuf;
-    guint out_watch;
-
-    /* Read under either BQL or out_lock, written with BQL+out_lock.  */
-    int mux_out;
-
     ReadLineState *rs;
     MonitorQMP qmp;
     gchar *mon_cpu_path;
@@ -224,6 +215,20 @@ struct Monitor {
     mon_cmd_t *cmd_table;
     QLIST_HEAD(,mon_fd_t) fds;
     QTAILQ_ENTRY(Monitor) entry;
+
+    /*
+     * The per-monitor lock. We can't access guest memory when holding
+     * the lock.
+     */
+    QemuMutex mon_lock;
+
+    /*
+     * Fields that are protected by the per-monitor lock.
+     */
+    QString *outbuf;
+    guint out_watch;
+    /* Read under either BQL or mon_lock, written with BQL+mon_lock.  */
+    int mux_out;
 };
 
 /* Let's add monitor global variables to this struct. */
@@ -366,14 +371,14 @@ static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
 {
     Monitor *mon = opaque;
 
-    qemu_mutex_lock(&mon->out_lock);
+    qemu_mutex_lock(&mon->mon_lock);
     mon->out_watch = 0;
     monitor_flush_locked(mon);
-    qemu_mutex_unlock(&mon->out_lock);
+    qemu_mutex_unlock(&mon->mon_lock);
     return FALSE;
 }
 
-/* Called with mon->out_lock held.  */
+/* Called with mon->mon_lock held.  */
 static void monitor_flush_locked(Monitor *mon)
 {
     int rc;
@@ -411,9 +416,9 @@ static void monitor_flush_locked(Monitor *mon)
 
 void monitor_flush(Monitor *mon)
 {
-    qemu_mutex_lock(&mon->out_lock);
+    qemu_mutex_lock(&mon->mon_lock);
     monitor_flush_locked(mon);
-    qemu_mutex_unlock(&mon->out_lock);
+    qemu_mutex_unlock(&mon->mon_lock);
 }
 
 /* flush at every end of line */
@@ -421,7 +426,7 @@ static void monitor_puts(Monitor *mon, const char *str)
 {
     char c;
 
-    qemu_mutex_lock(&mon->out_lock);
+    qemu_mutex_lock(&mon->mon_lock);
     for(;;) {
         c = *str++;
         if (c == '\0')
@@ -434,7 +439,7 @@ static void monitor_puts(Monitor *mon, const char *str)
             monitor_flush_locked(mon);
         }
     }
-    qemu_mutex_unlock(&mon->out_lock);
+    qemu_mutex_unlock(&mon->mon_lock);
 }
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
@@ -725,7 +730,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
                               bool use_io_thr)
 {
     memset(mon, 0, sizeof(Monitor));
-    qemu_mutex_init(&mon->out_lock);
+    qemu_mutex_init(&mon->mon_lock);
     qemu_mutex_init(&mon->qmp.qmp_queue_lock);
     mon->outbuf = qstring_new();
     /* Use *mon_cmds by default. */
@@ -745,7 +750,7 @@ static void monitor_data_destroy(Monitor *mon)
     }
     readline_free(mon->rs);
     qobject_unref(mon->outbuf);
-    qemu_mutex_destroy(&mon->out_lock);
+    qemu_mutex_destroy(&mon->mon_lock);
     qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
     monitor_qmp_cleanup_req_queue_locked(mon);
     monitor_qmp_cleanup_resp_queue_locked(mon);
@@ -777,13 +782,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     handle_hmp_command(&hmp, command_line);
     cur_mon = old_mon;
 
-    qemu_mutex_lock(&hmp.out_lock);
+    qemu_mutex_lock(&hmp.mon_lock);
     if (qstring_get_length(hmp.outbuf) > 0) {
         output = g_strdup(qstring_get_str(hmp.outbuf));
     } else {
         output = g_strdup("");
     }
-    qemu_mutex_unlock(&hmp.out_lock);
+    qemu_mutex_unlock(&hmp.mon_lock);
 
 out:
     monitor_data_destroy(&hmp);
@@ -4377,9 +4382,9 @@ static void monitor_event(void *opaque, int event)
 
     switch (event) {
     case CHR_EVENT_MUX_IN:
-        qemu_mutex_lock(&mon->out_lock);
+        qemu_mutex_lock(&mon->mon_lock);
         mon->mux_out = 0;
-        qemu_mutex_unlock(&mon->out_lock);
+        qemu_mutex_unlock(&mon->mon_lock);
         if (mon->reset_seen) {
             readline_restart(mon->rs);
             monitor_resume(mon);
@@ -4399,9 +4404,9 @@ static void monitor_event(void *opaque, int event)
         } else {
             atomic_inc(&mon->suspend_cnt);
         }
-        qemu_mutex_lock(&mon->out_lock);
+        qemu_mutex_lock(&mon->mon_lock);
         mon->mux_out = 1;
-        qemu_mutex_unlock(&mon->out_lock);
+        qemu_mutex_unlock(&mon->mon_lock);
         break;
 
     case CHR_EVENT_OPENED:
-- 
2.17.0

Re: [Qemu-devel] [PATCH v9 1/7] monitor: rename out_lock to mon_lock

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> The out_lock is protecting a few Monitor fields.  In the future the
> monitor code will start to run in multiple threads.  We are going to
> turn it into a bigger lock to protect not only the out buffer but also
> most of the rest.
>
> Since at it, rearrange the Monitor struct a bit.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v9 2/7] monitor: protect mon->fds with mon_lock

[Peter Xu]

mon->fds were protected by BQL.  Now protect it by mon_lock so that it
can even be used in monitor iothread.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 14c681dc8a..d6c3c08932 100644
--- a/monitor.c
+++ b/monitor.c
@@ -213,7 +213,6 @@ struct Monitor {
     BlockCompletionFunc *password_completion_cb;
     void *password_opaque;
     mon_cmd_t *cmd_table;
-    QLIST_HEAD(,mon_fd_t) fds;
     QTAILQ_ENTRY(Monitor) entry;
 
     /*
@@ -225,6 +224,7 @@ struct Monitor {
     /*
      * Fields that are protected by the per-monitor lock.
      */
+    QLIST_HEAD(, mon_fd_t) fds;
     QString *outbuf;
     guint out_watch;
     /* Read under either BQL or mon_lock, written with BQL+mon_lock.  */
@@ -2189,7 +2189,7 @@ static void hmp_acl_remove(Monitor *mon, const QDict *qdict)
 void qmp_getfd(const char *fdname, Error **errp)
 {
     mon_fd_t *monfd;
-    int fd;
+    int fd, tmp_fd;
 
     fd = qemu_chr_fe_get_msgfd(&cur_mon->chr);
     if (fd == -1) {
@@ -2204,13 +2204,17 @@ void qmp_getfd(const char *fdname, Error **errp)
         return;
     }
 
+    qemu_mutex_lock(&cur_mon->mon_lock);
     QLIST_FOREACH(monfd, &cur_mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
         }
 
-        close(monfd->fd);
+        tmp_fd = monfd->fd;
         monfd->fd = fd;
+        qemu_mutex_unlock(&cur_mon->mon_lock);
+        /* Make sure close() is out of critical section */
+        close(tmp_fd);
         return;
     }
 
@@ -2219,24 +2223,31 @@ void qmp_getfd(const char *fdname, Error **errp)
     monfd->fd = fd;
 
     QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
+    qemu_mutex_unlock(&cur_mon->mon_lock);
 }
 
 void qmp_closefd(const char *fdname, Error **errp)
 {
     mon_fd_t *monfd;
+    int tmp_fd;
 
+    qemu_mutex_lock(&cur_mon->mon_lock);
     QLIST_FOREACH(monfd, &cur_mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
         }
 
         QLIST_REMOVE(monfd, next);
-        close(monfd->fd);
+        tmp_fd = monfd->fd;
         g_free(monfd->name);
         g_free(monfd);
+        qemu_mutex_unlock(&cur_mon->mon_lock);
+        /* Make sure close() is out of critical section */
+        close(tmp_fd);
         return;
     }
 
+    qemu_mutex_unlock(&cur_mon->mon_lock);
     error_setg(errp, QERR_FD_NOT_FOUND, fdname);
 }
 
@@ -2244,6 +2255,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
 {
     mon_fd_t *monfd;
 
+    qemu_mutex_lock(&mon->mon_lock);
     QLIST_FOREACH(monfd, &mon->fds, next) {
         int fd;
 
@@ -2257,10 +2269,12 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
         QLIST_REMOVE(monfd, next);
         g_free(monfd->name);
         g_free(monfd);
+        qemu_mutex_unlock(&mon->mon_lock);
 
         return fd;
     }
 
+    qemu_mutex_unlock(&mon->mon_lock);
     error_setg(errp, "File descriptor named '%s' has not been found", fdname);
     return -1;
 }
-- 
2.17.0

[Qemu-devel] [PATCH v9 3/7] monitor: more comments on lock-free elements

[Peter Xu]

Add some explicit comments for both Readline and cpu_set/cpu_get helpers
that they do not need the mon_lock protection.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index d6c3c08932..f23178951e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -207,7 +207,15 @@ struct Monitor {
     int suspend_cnt;            /* Needs to be accessed atomically */
     bool skip_flush;
     bool use_io_thr;
+
+    /*
+     * State used only in the thread "owning" the monitor.
+     * If @use_io_thr, this is mon_global.mon_iothread.
+     * Else, it's the main thread.
+     * These members can be safely accessed without locks.
+     */
     ReadLineState *rs;
+
     MonitorQMP qmp;
     gchar *mon_cpu_path;
     BlockCompletionFunc *password_completion_cb;
@@ -1313,7 +1321,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
     cur_mon->qmp.commands = &qmp_commands;
 }
 
-/* set the current CPU defined by the user */
+/* Set the current CPU defined by the user. Callers must hold BQL. */
 int monitor_set_cpu(int cpu_index)
 {
     CPUState *cpu;
@@ -1327,6 +1335,7 @@ int monitor_set_cpu(int cpu_index)
     return 0;
 }
 
+/* Callers must hold BQL. */
 static CPUState *mon_get_cpu_sync(bool synchronize)
 {
     CPUState *cpu;
-- 
2.17.0

Re: [Qemu-devel] [PATCH v9 3/7] monitor: more comments on lock-free elements

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

On Tue, May 29, 2018 at 01:57:51PM +0800, Peter Xu wrote:
> Add some explicit comments for both Readline and cpu_set/cpu_get helpers
> that they do not need the mon_lock protection.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v9 3/7] monitor: more comments on lock-free elements

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Add some explicit comments for both Readline and cpu_set/cpu_get helpers
> that they do not need the mon_lock protection.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v9 4/7] monitor: fix comment for monitor_lock

[Peter Xu]

Fix typo in d622cb5879c.  Meanwhile move these variables close to each
other.  monitor_qapi_event_state can be declared static, add that.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index f23178951e..2504696d76 100644
--- a/monitor.c
+++ b/monitor.c
@@ -267,10 +267,11 @@ typedef struct QMPRequest QMPRequest;
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
-/* Protects mon_list, monitor_event_state.  */
+/* Protects mon_list, monitor_qapi_event_state.  */
 static QemuMutex monitor_lock;
-
+static GHashTable *monitor_qapi_event_state;
 static QTAILQ_HEAD(mon_list, Monitor) mon_list;
+
 static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
 static int mon_refcount;
 
@@ -572,8 +573,6 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
 };
 
-GHashTable *monitor_qapi_event_state;
-
 /*
  * Emits the event to every monitor instance, @event is only used for trace
  * Called with monitor_lock held.
-- 
2.17.0

Re: [Qemu-devel] [PATCH v9 4/7] monitor: fix comment for monitor_lock

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

On Tue, May 29, 2018 at 01:57:52PM +0800, Peter Xu wrote:
> Fix typo in d622cb5879c.  Meanwhile move these variables close to each
> other.  monitor_qapi_event_state can be declared static, add that.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v9 4/7] monitor: fix comment for monitor_lock

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Fix typo in d622cb5879c.  Meanwhile move these variables close to each
> other.  monitor_qapi_event_state can be declared static, add that.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v9 5/7] monitor: remove event_clock_type

[Peter Xu]

Instead, use a dynamic function to detect which clock we'll use.  The
problem is that the old code will let monitor initialization depends on
qtest_enabled().  After this change, we don't have such a dependency any
more.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2504696d76..bd9ab5597f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -282,8 +282,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);
 
@@ -310,6 +308,15 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
     return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
 }
 
+static inline QEMUClockType monitor_get_clock(void)
+{
+    if (qtest_enabled()) {
+        return QEMU_CLOCK_VIRTUAL;
+    } else {
+        return QEMU_CLOCK_REALTIME;
+    }
+}
+
 /**
  * Is the current monitor, if any, a QMP monitor?
  */
@@ -633,7 +640,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);
 
@@ -641,7 +648,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);
@@ -666,7 +673,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);
@@ -722,10 +729,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);
-- 
2.17.0

Re: [Qemu-devel] [PATCH v9 5/7] monitor: remove event_clock_type

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]

On Tue, May 29, 2018 at 01:57:53PM +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 depends on
> qtest_enabled().  After this change, we don't have such a dependency any
> more.

There is a hidden dependency:

  monitor_get_clock() returns the wrong value before main() has
  processed command-line arguments.

Where is the guarantee that monitor_get_clock() is never called too
early?

At the least, monitor_get_clock() should call abort(3) if invoked too
early.  Even better would be an interface that cannot be used
incorrectly.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v9 5/7] monitor: remove event_clock_type

[Peter Xu]

On Wed, May 30, 2018 at 05:35:52PM +0100, Stefan Hajnoczi wrote:
> On Tue, May 29, 2018 at 01:57:53PM +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 depends on
> > qtest_enabled().  After this change, we don't have such a dependency any
> > more.
> 
> There is a hidden dependency:
> 
>   monitor_get_clock() returns the wrong value before main() has
>   processed command-line arguments.

To be more explicit:

    monitor_get_clock() returns the wrong value before accelerator is
    correctly setup (in configure_accelerator()).

Since the only thing that matters here is whether we're using the
qtest accelerator.

> 
> Where is the guarantee that monitor_get_clock() is never called too
> early?

You are right, there is no guarantee except from programming POV.
It's only used in:

  monitor_qapi_event_queue
  monitor_qapi_event_handler

These two functions will never be called until accelerator is setup.

> 
> At the least, monitor_get_clock() should call abort(3) if invoked too
> early.  Even better would be an interface that cannot be used
> incorrectly.

Maybe then I should export the accel_initialised variable in
configure_accelerator() and then I assert with that.  But that'll
further expand the series a bit.

Or, I can also mention above in the commit message to explain that a
bit.

What would you prefer?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v9 5/7] monitor: remove event_clock_type

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

On Thu, May 31, 2018 at 12:11:47PM +0800, Peter Xu wrote:
> On Wed, May 30, 2018 at 05:35:52PM +0100, Stefan Hajnoczi wrote:
> > On Tue, May 29, 2018 at 01:57:53PM +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 depends on
> > > qtest_enabled().  After this change, we don't have such a dependency any
> > > more.
> > 
> > There is a hidden dependency:
> > 
> >   monitor_get_clock() returns the wrong value before main() has
> >   processed command-line arguments.
> 
> To be more explicit:
> 
>     monitor_get_clock() returns the wrong value before accelerator is
>     correctly setup (in configure_accelerator()).
> 
> Since the only thing that matters here is whether we're using the
> qtest accelerator.
> 
> > 
> > Where is the guarantee that monitor_get_clock() is never called too
> > early?
> 
> You are right, there is no guarantee except from programming POV.
> It's only used in:
> 
>   monitor_qapi_event_queue
>   monitor_qapi_event_handler
> 
> These two functions will never be called until accelerator is setup.
> 
> > 
> > At the least, monitor_get_clock() should call abort(3) if invoked too
> > early.  Even better would be an interface that cannot be used
> > incorrectly.
> 
> Maybe then I should export the accel_initialised variable in
> configure_accelerator() and then I assert with that.  But that'll
> further expand the series a bit.
> 
> Or, I can also mention above in the commit message to explain that a
> bit.

Documentation is okay, but please do it in the code, not the commit
message.  That way anyone looking at monitor_get_clock() will be aware
of the constraint.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v9 5/7] monitor: remove event_clock_type

[Peter Xu]

On Thu, May 31, 2018 at 09:23:24AM +0100, Stefan Hajnoczi wrote:
> On Thu, May 31, 2018 at 12:11:47PM +0800, Peter Xu wrote:
> > On Wed, May 30, 2018 at 05:35:52PM +0100, Stefan Hajnoczi wrote:
> > > On Tue, May 29, 2018 at 01:57:53PM +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 depends on
> > > > qtest_enabled().  After this change, we don't have such a dependency any
> > > > more.
> > > 
> > > There is a hidden dependency:
> > > 
> > >   monitor_get_clock() returns the wrong value before main() has
> > >   processed command-line arguments.
> > 
> > To be more explicit:
> > 
> >     monitor_get_clock() returns the wrong value before accelerator is
> >     correctly setup (in configure_accelerator()).
> > 
> > Since the only thing that matters here is whether we're using the
> > qtest accelerator.
> > 
> > > 
> > > Where is the guarantee that monitor_get_clock() is never called too
> > > early?
> > 
> > You are right, there is no guarantee except from programming POV.
> > It's only used in:
> > 
> >   monitor_qapi_event_queue
> >   monitor_qapi_event_handler
> > 
> > These two functions will never be called until accelerator is setup.
> > 
> > > 
> > > At the least, monitor_get_clock() should call abort(3) if invoked too
> > > early.  Even better would be an interface that cannot be used
> > > incorrectly.
> > 
> > Maybe then I should export the accel_initialised variable in
> > configure_accelerator() and then I assert with that.  But that'll
> > further expand the series a bit.
> > 
> > Or, I can also mention above in the commit message to explain that a
> > bit.
> 
> Documentation is okay, but please do it in the code, not the commit
> message.  That way anyone looking at monitor_get_clock() will be aware
> of the constraint.

Sure.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v9 5/7] monitor: remove event_clock_type

[Markus Armbruster]

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 depends on
> qtest_enabled().  After this change, we don't have such a dependency any
> more.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 2504696d76..bd9ab5597f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -282,8 +282,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);
>  
> @@ -310,6 +308,15 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
>      return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
>  }
>  
> +static inline QEMUClockType monitor_get_clock(void)
> +{
> +    if (qtest_enabled()) {
> +        return QEMU_CLOCK_VIRTUAL;
> +    } else {
> +        return QEMU_CLOCK_REALTIME;
> +    }

Suggest the more laconic

       return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;

A comment explaining why we want QEMU_CLOCK_VIRTUAL would be nice.

> +}
> +
>  /**
>   * Is the current monitor, if any, a QMP monitor?
>   */
[...]

Re: [Qemu-devel] [PATCH v9 5/7] monitor: remove event_clock_type

[Peter Xu]

On Thu, Jun 07, 2018 at 04:32:54PM +0200, Markus Armbruster wrote:
> 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 depends on
> > qtest_enabled().  After this change, we don't have such a dependency any
> > more.
> >
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 2504696d76..bd9ab5597f 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -282,8 +282,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);
> >  
> > @@ -310,6 +308,15 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
> >      return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
> >  }
> >  
> > +static inline QEMUClockType monitor_get_clock(void)
> > +{
> > +    if (qtest_enabled()) {
> > +        return QEMU_CLOCK_VIRTUAL;
> > +    } else {
> > +        return QEMU_CLOCK_REALTIME;
> > +    }
> 
> Suggest the more laconic
> 
>        return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
> 
> A comment explaining why we want QEMU_CLOCK_VIRTUAL would be nice.

Will do.

-- 
Peter Xu

[Qemu-devel] [PATCH v9 6/7] monitor: move init global earlier

[Peter Xu]

Before this patch, monitor fd helpers might be called even earlier than
monitor_init_globals().  This can be problematic.

After previous work, now monitor_init_globals() does not depend on
accelerator initialization any more.  Call it earlier (before CLI
parsing; that's where the monitor APIs might be called) to make sure it
is called before any of the monitor APIs.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 vl.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/vl.c b/vl.c
index 038d7f8042..97b43b399f 100644
--- a/vl.c
+++ b/vl.c
@@ -3074,6 +3074,7 @@ int main(int argc, char **argv, char **envp)
 
     runstate_init();
     postcopy_infrastructure_init();
+    monitor_init_globals();
 
     if (qcrypto_init(&err) < 0) {
         error_reportf_err(err, "cannot initialize crypto: ");
@@ -4490,12 +4491,6 @@ int main(int argc, char **argv, char **envp)
     default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
     default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
 
-    /*
-     * Note: qtest_enabled() (which is used in monitor_qapi_event_init())
-     * depends on configure_accelerator() above.
-     */
-    monitor_init_globals();
-
     if (qemu_opts_foreach(qemu_find_opts("mon"),
                           mon_init_func, NULL, NULL)) {
         exit(1);
-- 
2.17.0

Re: [Qemu-devel] [PATCH v9 6/7] monitor: move init global earlier

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

On Tue, May 29, 2018 at 01:57:54PM +0800, Peter Xu wrote:
> Before this patch, monitor fd helpers might be called even earlier than
> monitor_init_globals().  This can be problematic.
> 
> After previous work, now monitor_init_globals() does not depend on
> accelerator initialization any more.  Call it earlier (before CLI
> parsing; that's where the monitor APIs might be called) to make sure it
> is called before any of the monitor APIs.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  vl.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v9 6/7] monitor: move init global earlier

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Before this patch, monitor fd helpers might be called even earlier than
> monitor_init_globals().  This can be problematic.
>
> After previous work, now monitor_init_globals() does not depend on
> accelerator initialization any more.  Call it earlier (before CLI
> parsing; that's where the monitor APIs might be called) to make sure it
> is called before any of the monitor APIs.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v9 7/7] monitor: add lock to protect mon_fdsets

[Peter Xu]

Introduce a new global big lock for mon_fdsets.  Take it where needed.

The monitor_fdset_get_fd() handling is a bit tricky: now we need to call
qemu_mutex_unlock() which might pollute errno, so we need to make sure
the correct errno be passed up to the callers.  To make things simpler,
we let monitor_fdset_get_fd() return the -errno directly when error
happens, then in qemu_open() we move it back into errno.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c     | 52 +++++++++++++++++++++++++++++++++++++++++----------
 stubs/fdset.c |  2 +-
 util/osdep.c  |  3 ++-
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index bd9ab5597f..09902a58d5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -272,7 +272,10 @@ static QemuMutex monitor_lock;
 static GHashTable *monitor_qapi_event_state;
 static QTAILQ_HEAD(mon_list, Monitor) mon_list;
 
+/* Protects mon_fdsets */
+static QemuMutex mon_fdsets_lock;
 static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
+
 static int mon_refcount;
 
 static mon_cmd_t mon_cmds[];
@@ -2317,9 +2320,11 @@ static void monitor_fdsets_cleanup(void)
     MonFdset *mon_fdset;
     MonFdset *mon_fdset_next;
 
+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH_SAFE(mon_fdset, &mon_fdsets, next, mon_fdset_next) {
         monitor_fdset_cleanup(mon_fdset);
     }
+    qemu_mutex_unlock(&mon_fdsets_lock);
 }
 
 AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
@@ -2354,6 +2359,7 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
     MonFdsetFd *mon_fdset_fd;
     char fd_str[60];
 
+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
         if (mon_fdset->id != fdset_id) {
             continue;
@@ -2373,10 +2379,12 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
             goto error;
         }
         monitor_fdset_cleanup(mon_fdset);
+        qemu_mutex_unlock(&mon_fdsets_lock);
         return;
     }
 
 error:
+    qemu_mutex_unlock(&mon_fdsets_lock);
     if (has_fd) {
         snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64 ", fd:%" PRId64,
                  fdset_id, fd);
@@ -2392,6 +2400,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
     MonFdsetFd *mon_fdset_fd;
     FdsetInfoList *fdset_list = NULL;
 
+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
         FdsetInfoList *fdset_info = g_malloc0(sizeof(*fdset_info));
         FdsetFdInfoList *fdsetfd_list = NULL;
@@ -2421,6 +2430,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
         fdset_info->next = fdset_list;
         fdset_list = fdset_info;
     }
+    qemu_mutex_unlock(&mon_fdsets_lock);
 
     return fdset_list;
 }
@@ -2433,6 +2443,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
     MonFdsetFd *mon_fdset_fd;
     AddfdInfo *fdinfo;
 
+    qemu_mutex_lock(&mon_fdsets_lock);
     if (has_fdset_id) {
         QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
             /* Break if match found or match impossible due to ordering by ID */
@@ -2453,6 +2464,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
             if (fdset_id < 0) {
                 error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id",
                            "a non-negative value");
+                qemu_mutex_unlock(&mon_fdsets_lock);
                 return NULL;
             }
             /* Use specified fdset ID */
@@ -2503,16 +2515,21 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
     fdinfo->fdset_id = mon_fdset->id;
     fdinfo->fd = mon_fdset_fd->fd;
 
+    qemu_mutex_unlock(&mon_fdsets_lock);
     return fdinfo;
 }
 
 int monitor_fdset_get_fd(int64_t fdset_id, int flags)
 {
-#ifndef _WIN32
+#ifdef _WIN32
+    return -ENOENT;
+#else
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd;
     int mon_fd_flags;
+    int ret;
 
+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
         if (mon_fdset->id != fdset_id) {
             continue;
@@ -2520,20 +2537,24 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
         QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
             mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
             if (mon_fd_flags == -1) {
-                return -1;
+                ret = -errno;
+                goto out;
             }
 
             if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
-                return mon_fdset_fd->fd;
+                ret = mon_fdset_fd->fd;
+                goto out;
             }
         }
-        errno = EACCES;
-        return -1;
+        ret = -EACCES;
+        goto out;
     }
-#endif
+    ret = -ENOENT;
 
-    errno = ENOENT;
-    return -1;
+out:
+    qemu_mutex_unlock(&mon_fdsets_lock);
+    return ret;
+#endif
 }
 
 int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
@@ -2541,20 +2562,25 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd_dup;
 
+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
         if (mon_fdset->id != fdset_id) {
             continue;
         }
         QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
             if (mon_fdset_fd_dup->fd == dup_fd) {
-                return -1;
+                goto err;
             }
         }
         mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
         mon_fdset_fd_dup->fd = dup_fd;
         QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
+        qemu_mutex_unlock(&mon_fdsets_lock);
         return 0;
     }
+
+err:
+    qemu_mutex_unlock(&mon_fdsets_lock);
     return -1;
 }
 
@@ -2563,6 +2589,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd_dup;
 
+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
         QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
             if (mon_fdset_fd_dup->fd == dup_fd) {
@@ -2571,13 +2598,17 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
                     if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
                         monitor_fdset_cleanup(mon_fdset);
                     }
-                    return -1;
+                    goto err;
                 } else {
+                    qemu_mutex_unlock(&mon_fdsets_lock);
                     return mon_fdset->id;
                 }
             }
         }
     }
+
+err:
+    qemu_mutex_unlock(&mon_fdsets_lock);
     return -1;
 }
 
@@ -4511,6 +4542,7 @@ void monitor_init_globals(void)
     monitor_qapi_event_init();
     sortcmdlist();
     qemu_mutex_init(&monitor_lock);
+    qemu_mutex_init(&mon_fdsets_lock);
     monitor_iothread_init();
 }
 
diff --git a/stubs/fdset.c b/stubs/fdset.c
index 6020cf28c8..4f3edf2ea4 100644
--- a/stubs/fdset.c
+++ b/stubs/fdset.c
@@ -14,7 +14,7 @@ int monitor_fdset_dup_fd_find(int dup_fd)
 
 int monitor_fdset_get_fd(int64_t fdset_id, int flags)
 {
-    return -1;
+    return -ENOENT;
 }
 
 void monitor_fdset_dup_fd_remove(int dupfd)
diff --git a/util/osdep.c b/util/osdep.c
index a73de0e1ba..ea51d500b6 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -302,7 +302,8 @@ int qemu_open(const char *name, int flags, ...)
         }
 
         fd = monitor_fdset_get_fd(fdset_id, flags);
-        if (fd == -1) {
+        if (fd < 0) {
+            errno = -fd;
             return -1;
         }
 
-- 
2.17.0

Re: [Qemu-devel] [PATCH v9 7/7] monitor: add lock to protect mon_fdsets

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Tue, May 29, 2018 at 01:57:55PM +0800, Peter Xu wrote:
> Introduce a new global big lock for mon_fdsets.  Take it where needed.
> 
> The monitor_fdset_get_fd() handling is a bit tricky: now we need to call
> qemu_mutex_unlock() which might pollute errno, so we need to make sure
> the correct errno be passed up to the callers.  To make things simpler,
> we let monitor_fdset_get_fd() return the -errno directly when error
> happens, then in qemu_open() we move it back into errno.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c     | 52 +++++++++++++++++++++++++++++++++++++++++----------
>  stubs/fdset.c |  2 +-
>  util/osdep.c  |  3 ++-
>  3 files changed, 45 insertions(+), 12 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v9 7/7] monitor: add lock to protect mon_fdsets

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Introduce a new global big lock for mon_fdsets.  Take it where needed.
>
> The monitor_fdset_get_fd() handling is a bit tricky: now we need to call
> qemu_mutex_unlock() which might pollute errno, so we need to make sure
> the correct errno be passed up to the callers.  To make things simpler,
> we let monitor_fdset_get_fd() return the -errno directly when error
> happens, then in qemu_open() we move it back into errno.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>