[Qemu-devel] [PATCH v2 0/3] monitor: let Monitor be thread safe

[Qemu-devel] [PATCH v2 0/3] monitor: let Monitor be thread safe

[Peter Xu]

v2:
- cc correct people... sorry.

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.

Patch 1 renames the old out_lock to mon_lock, so that it can cover
more things.

Patch 2 uses the mon_lock to protect Monitor object, at least all the
APIs exported in monitor.h

Patch 3 introduces mon_fdsets_lock to protect mon_fdsets global.

Tests: x86 only, make check, raw iotests, windows build.

Please review.  Thanks,

Peter Xu (3):
  monitor: rename out_lock to mon_lock
  monitor: take mon_lock where proper
  monitor: add lock to protect mon_fdsets

 monitor.c | 120 +++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 87 insertions(+), 33 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/3] monitor: rename out_lock to mon_lock

[Peter Xu]

The out_lock was only protecting out buffers.  In the future the monitor
code will start to run in multiple threads.  We turn it into a bigger
lock to protect not only the out buffer but also all the rest.  We split
this lock until necessary.  So far I don't see a reason to complicate
lock usage for monitors.

Since at it, arrange the Monitor struct a bit.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/monitor.c b/monitor.c
index 39f8ee17ba..c93aa4e22b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -202,20 +202,17 @@ typedef struct {
 
 struct Monitor {
     CharBackend chr;
+    /* We can't access guest memory when holding the lock */
+    QemuMutex mon_lock;
     int reset_seen;
     int flags;
     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.  */
+    /* Read under either BQL or mon_lock, written with BQL+mon_lock.  */
     int mux_out;
-
     ReadLineState *rs;
     MonitorQMP qmp;
     gchar *mon_cpu_path;
@@ -366,14 +363,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 +408,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 +418,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 +431,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)
@@ -728,7 +725,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. */
@@ -748,7 +745,7 @@ static void monitor_data_destroy(Monitor *mon)
     }
     readline_free(mon->rs);
     QDECREF(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);
@@ -780,13 +777,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);
@@ -4383,9 +4380,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);
@@ -4405,9 +4402,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.14.3

[Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper

[Peter Xu]

Went through all the montior.h APIs to make sure existing Monitor
related APIs will always take the new monitor lock when necessary.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index c93aa4e22b..f4951cafbc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
     if (!mon->rs)
         return;
 
+    qemu_mutex_lock(&mon->mon_lock);
     readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
     if (show_prompt)
         readline_show_prompt(mon->rs);
+    qemu_mutex_unlock(&mon->mon_lock);
 }
 
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
                           void *opaque)
 {
     if (mon->rs) {
+        qemu_mutex_lock(&mon->mon_lock);
         readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
+        qemu_mutex_unlock(&mon->mon_lock);
         /* prompt is printed on return from the command handler */
         return 0;
     } else {
@@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
     cur_mon->qmp.commands = &qmp_commands;
 }
 
-/* set the current CPU defined by the user */
-int monitor_set_cpu(int cpu_index)
+static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)
 {
     CPUState *cpu;
 
@@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
     if (cpu == NULL) {
         return -1;
     }
-    g_free(cur_mon->mon_cpu_path);
-    cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
+    g_free(mon->mon_cpu_path);
+    mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
     return 0;
 }
 
+/* set the current CPU defined by the user */
+int monitor_set_cpu(int cpu_index)
+{
+    int ret;
+
+    qemu_mutex_lock(&cur_mon->mon_lock);
+    ret = monitor_set_cpu_locked(cur_mon, cpu_index);
+    qemu_mutex_unlock(&cur_mon->mon_lock);
+
+    return ret;
+}
+
 static CPUState *mon_get_cpu_sync(bool synchronize)
 {
     CPUState *cpu;
 
+    qemu_mutex_lock(&cur_mon->mon_lock);
     if (cur_mon->mon_cpu_path) {
         cpu = (CPUState *) object_resolve_path_type(cur_mon->mon_cpu_path,
                                                     TYPE_CPU, NULL);
@@ -1336,11 +1352,14 @@ static CPUState *mon_get_cpu_sync(bool synchronize)
     }
     if (!cur_mon->mon_cpu_path) {
         if (!first_cpu) {
+            qemu_mutex_unlock(&cur_mon->mon_lock);
             return NULL;
         }
-        monitor_set_cpu(first_cpu->cpu_index);
+        monitor_set_cpu_locked(cur_mon, first_cpu->cpu_index);
         cpu = first_cpu;
     }
+    qemu_mutex_unlock(&cur_mon->mon_lock);
+
     if (synchronize) {
         cpu_synchronize_state(cpu);
     }
@@ -2239,6 +2258,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;
 
@@ -2252,9 +2272,10 @@ 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.14.3

[Qemu-devel] [PATCH v2 3/3] monitor: add lock to protect mon_fdsets

[Peter Xu]

Similar to previous patch, but introduce a new global big lock for
mon_fdsets.  Take it where needed.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index f4951cafbc..40b5b56f66 100644
--- a/monitor.c
+++ b/monitor.c
@@ -254,6 +254,9 @@ typedef struct QMPRequest QMPRequest;
 /* Protects mon_list, monitor_event_state.  */
 static QemuMutex monitor_lock;
 
+/* Protects mon_fdsets */
+static QemuMutex mon_fdsets_lock;
+
 static QTAILQ_HEAD(mon_list, Monitor) mon_list;
 static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
 static int mon_refcount;
@@ -270,6 +273,16 @@ static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME;
 static void monitor_command_cb(void *opaque, const char *cmdline,
                                void *readline_opaque);
 
+/*
+ * This lock can be used very early, even during param parsing.
+ * Meanwhile it can also be used even at the end of main.  Let's keep
+ * it initialized for the whole lifecycle of QEMU.
+ */
+static void __attribute__((constructor)) mon_fdsets_lock_init(void)
+{
+    qemu_mutex_init(&mon_fdsets_lock);
+}
+
 /**
  * Is @mon a QMP monitor?
  */
@@ -2308,9 +2321,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,
@@ -2345,6 +2360,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;
@@ -2364,10 +2380,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);
@@ -2383,6 +2401,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;
@@ -2412,6 +2431,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;
 }
@@ -2424,6 +2444,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 */
@@ -2444,6 +2465,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 */
@@ -2494,6 +2516,7 @@ 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;
 }
 
@@ -2531,29 +2554,38 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
 {
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd_dup;
+    int ret = -1;
 
+    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;
+                ret = -1;
+                goto out;
             }
         }
         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);
-        return 0;
+        ret = 0;
+        break;
     }
-    return -1;
+
+out:
+    qemu_mutex_unlock(&mon_fdsets_lock);
+    return ret;
 }
 
 static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
 {
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd_dup;
+    int ret = -1;
 
+    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) {
@@ -2562,14 +2594,18 @@ 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;
+                    ret = -1;
+                    goto out;
                 } else {
-                    return mon_fdset->id;
+                    ret = mon_fdset->id;
+                    goto out;
                 }
             }
         }
     }
-    return -1;
+out:
+    qemu_mutex_unlock(&mon_fdsets_lock);
+    return ret;
 }
 
 int monitor_fdset_dup_fd_find(int dup_fd)
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 1/3] monitor: rename out_lock to mon_lock

[Stefan Hajnoczi]

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

On Wed, Apr 18, 2018 at 05:02:37PM +0800, Peter Xu wrote:
> The out_lock was only protecting out buffers.  In the future the monitor
> code will start to run in multiple threads.  We turn it into a bigger
> lock to protect not only the out buffer but also all the rest.  We split
> this lock until necessary.  So far I don't see a reason to complicate
> lock usage for monitors.
> 
> Since at it, arrange the Monitor struct a bit.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 39f8ee17ba..c93aa4e22b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -202,20 +202,17 @@ typedef struct {
>  
>  struct Monitor {
>      CharBackend chr;
> +    /* We can't access guest memory when holding the lock */
> +    QemuMutex mon_lock;

Please document which field this lock protects.  For example, outbuf and
out_watch need a comment.

>      int reset_seen;
>      int flags;
>      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;

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

Re: [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper

[Stefan Hajnoczi]

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

On Wed, Apr 18, 2018 at 05:02:38PM +0800, Peter Xu wrote:
> diff --git a/monitor.c b/monitor.c
> index c93aa4e22b..f4951cafbc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
>      if (!mon->rs)
>          return;
>  
> +    qemu_mutex_lock(&mon->mon_lock);
>      readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
>      if (show_prompt)
>          readline_show_prompt(mon->rs);
> +    qemu_mutex_unlock(&mon->mon_lock);
>  }
>  
>  int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
>                            void *opaque)
>  {
>      if (mon->rs) {
> +        qemu_mutex_lock(&mon->mon_lock);
>          readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
> +        qemu_mutex_unlock(&mon->mon_lock);
>          /* prompt is printed on return from the command handler */
>          return 0;
>      } else {

I'm not sure why the lock is being used around readline_start() and
readline_show_prompt().  There are other readline_*() callers who do not
take the lock, which is suspicious.

Can you explain the purpose of this?

> @@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
>      cur_mon->qmp.commands = &qmp_commands;
>  }
>  
> -/* set the current CPU defined by the user */
> -int monitor_set_cpu(int cpu_index)
> +static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)

This function requires the BQL since qemu_get_cpu() accesses the cpus
list without acquiring qemu_cpu_list_lock.

Two options:
1. Document that monitor_set_cpu() must be called with the BQL held.
2. Audit qemu_cpu_list_lock to check that it meets the out-of-band
   monitor code requirements, document that qemu_cpu_list_lock code must
   follow out-of-band monitor code requirements, and then take the lock.

#1 is more practical since we will probably never need to call
monitor_set_cpu() from out-of-band monitor code.  Anyway, in that case
mon_lock is not needed unless there is a mon field that needs to be
protected.

>  {
>      CPUState *cpu;
>  
> @@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
>      if (cpu == NULL) {
>          return -1;
>      }
> -    g_free(cur_mon->mon_cpu_path);
> -    cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> +    g_free(mon->mon_cpu_path);
> +    mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
>      return 0;
>  }
>  
> +/* set the current CPU defined by the user */
> +int monitor_set_cpu(int cpu_index)
> +{
> +    int ret;
> +
> +    qemu_mutex_lock(&cur_mon->mon_lock);
> +    ret = monitor_set_cpu_locked(cur_mon, cpu_index);
> +    qemu_mutex_unlock(&cur_mon->mon_lock);
> +
> +    return ret;
> +}
> +
>  static CPUState *mon_get_cpu_sync(bool synchronize)
>  {

This function calls monitor_set_cpu() so it must be called from the BQL.
The locking changes are probably not needed.  This function just needs
to be documented as BQL-only.

> @@ -2239,6 +2258,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;
>  
> @@ -2252,9 +2272,10 @@ 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);

What about all the other mon->fds users?  They need to lock too,
otherwise we will QLIST_REMOVE() an fd while they are accessing the
list!

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

Re: [Qemu-devel] [PATCH v2 3/3] monitor: add lock to protect mon_fdsets

[Stefan Hajnoczi]

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

On Wed, Apr 18, 2018 at 05:02:39PM +0800, Peter Xu wrote:
> Similar to previous patch, but introduce a new global big lock for
> mon_fdsets.  Take it where needed.

Looks like monitor_fdset_get_fd() is missing.

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

Re: [Qemu-devel] [PATCH v2 1/3] monitor: rename out_lock to mon_lock

[Peter Xu]

On Mon, Apr 30, 2018 at 10:56:49AM +0100, Stefan Hajnoczi wrote:
> On Wed, Apr 18, 2018 at 05:02:37PM +0800, Peter Xu wrote:
> > The out_lock was only protecting out buffers.  In the future the monitor
> > code will start to run in multiple threads.  We turn it into a bigger
> > lock to protect not only the out buffer but also all the rest.  We split
> > this lock until necessary.  So far I don't see a reason to complicate
> > lock usage for monitors.
> > 
> > Since at it, arrange the Monitor struct a bit.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 39 ++++++++++++++++++---------------------
> >  1 file changed, 18 insertions(+), 21 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 39f8ee17ba..c93aa4e22b 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -202,20 +202,17 @@ typedef struct {
> >  
> >  struct Monitor {
> >      CharBackend chr;
> > +    /* We can't access guest memory when holding the lock */
> > +    QemuMutex mon_lock;
> 
> Please document which field this lock protects.  For example, outbuf and
> out_watch need a comment.

Sure!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper

[Peter Xu]

On Mon, Apr 30, 2018 at 11:10:50AM +0100, Stefan Hajnoczi wrote:
> On Wed, Apr 18, 2018 at 05:02:38PM +0800, Peter Xu wrote:
> > diff --git a/monitor.c b/monitor.c
> > index c93aa4e22b..f4951cafbc 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
> >      if (!mon->rs)
> >          return;
> >  
> > +    qemu_mutex_lock(&mon->mon_lock);
> >      readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
> >      if (show_prompt)
> >          readline_show_prompt(mon->rs);
> > +    qemu_mutex_unlock(&mon->mon_lock);
> >  }
> >  
> >  int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
> >                            void *opaque)
> >  {
> >      if (mon->rs) {
> > +        qemu_mutex_lock(&mon->mon_lock);
> >          readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
> > +        qemu_mutex_unlock(&mon->mon_lock);
> >          /* prompt is printed on return from the command handler */
> >          return 0;
> >      } else {
> 
> I'm not sure why the lock is being used around readline_start() and
> readline_show_prompt().  There are other readline_*() callers who do not
> take the lock, which is suspicious.
> 
> Can you explain the purpose of this?
> 
> > @@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
> >      cur_mon->qmp.commands = &qmp_commands;
> >  }
> >  
> > -/* set the current CPU defined by the user */
> > -int monitor_set_cpu(int cpu_index)
> > +static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)
> 
> This function requires the BQL since qemu_get_cpu() accesses the cpus
> list without acquiring qemu_cpu_list_lock.
> 
> Two options:
> 1. Document that monitor_set_cpu() must be called with the BQL held.
> 2. Audit qemu_cpu_list_lock to check that it meets the out-of-band
>    monitor code requirements, document that qemu_cpu_list_lock code must
>    follow out-of-band monitor code requirements, and then take the lock.
> 
> #1 is more practical since we will probably never need to call
> monitor_set_cpu() from out-of-band monitor code.  Anyway, in that case
> mon_lock is not needed unless there is a mon field that needs to be
> protected.

You are right.

After a second thought I think readline is not needed to be protected.
IMHO it's only used in parsing phase, so actually we don't have
multi-threading issue with that (parsing is either happening in main
thread only, or monitor iothread only).

So I'll drop all the readline_* protections, and add a comment for
monitor_set_cpu() on BQL.

> 
> >  {
> >      CPUState *cpu;
> >  
> > @@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
> >      if (cpu == NULL) {
> >          return -1;
> >      }
> > -    g_free(cur_mon->mon_cpu_path);
> > -    cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> > +    g_free(mon->mon_cpu_path);
> > +    mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> >      return 0;
> >  }
> >  
> > +/* set the current CPU defined by the user */
> > +int monitor_set_cpu(int cpu_index)
> > +{
> > +    int ret;
> > +
> > +    qemu_mutex_lock(&cur_mon->mon_lock);
> > +    ret = monitor_set_cpu_locked(cur_mon, cpu_index);
> > +    qemu_mutex_unlock(&cur_mon->mon_lock);
> > +
> > +    return ret;
> > +}
> > +
> >  static CPUState *mon_get_cpu_sync(bool synchronize)
> >  {
> 
> This function calls monitor_set_cpu() so it must be called from the BQL.
> The locking changes are probably not needed.  This function just needs
> to be documented as BQL-only.

Yes.  Will do.

> 
> > @@ -2239,6 +2258,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;
> >  
> > @@ -2252,9 +2272,10 @@ 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);
> 
> What about all the other mon->fds users?  They need to lock too,
> otherwise we will QLIST_REMOVE() an fd while they are accessing the
> list!

Indeed!  I think I'll drop most of this patch, only add protection for
mon->fds, and add those comments that you suggested.  They make sense
to me.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 3/3] monitor: add lock to protect mon_fdsets

[Peter Xu]

On Mon, Apr 30, 2018 at 11:21:26AM +0100, Stefan Hajnoczi wrote:
> On Wed, Apr 18, 2018 at 05:02:39PM +0800, Peter Xu wrote:
> > Similar to previous patch, but introduce a new global big lock for
> > mon_fdsets.  Take it where needed.
> 
> Looks like monitor_fdset_get_fd() is missing.

Yes.  Fixing that up.  Thanks,

-- 
Peter Xu