[Qemu-devel] [PATCH v4 0/4] monitor: let Monitor be thread safe

[Qemu-devel] [PATCH v4 0/4] monitor: let Monitor be thread safe

[Peter Xu]

v4:
- fix a s/cur_mon/mon/ typo

v3:
- add comment for fields that are protected by monitor lock [Stefan]
- drop most of patch 2, only keep the protections for mon->fds [Stefan]
- add one trivial patch to add some more comments for either readline
  and cpu_get/cpu_set [Stefan]
- add protection for monitor_fdset_get_fd() [Stefan]

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.

Please review.  Thanks,

Peter Xu (4):
  monitor: rename out_lock to mon_lock
  monitor: protect mon->fds with mon_lock
  monitor: more comments on lock-free fleids/funcs
  monitor: add lock to protect mon_fdsets

 monitor.c | 132 ++++++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 95 insertions(+), 37 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v4 1/4] monitor: rename out_lock to mon_lock

[Peter Xu]

The out_lock was only protecting a few Monitor fields.  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.
For now we share the lock.  We can split the lock when needed.

Since at it, arrange the Monitor struct a bit.

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 39f8ee17ba..48882d28ae 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)
@@ -728,7 +733,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 +753,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 +785,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 +4388,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 +4410,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 v4 2/4] 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.

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 48882d28ae..9f361ec21e 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.  */
@@ -2207,6 +2207,7 @@ 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;
@@ -2214,6 +2215,7 @@ void qmp_getfd(const char *fdname, Error **errp)
 
         close(monfd->fd);
         monfd->fd = fd;
+        qemu_mutex_unlock(&cur_mon->mon_lock);
         return;
     }
 
@@ -2222,12 +2224,14 @@ 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;
 
+    qemu_mutex_lock(&cur_mon->mon_lock);
     QLIST_FOREACH(monfd, &cur_mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
@@ -2237,9 +2241,11 @@ void qmp_closefd(const char *fdname, Error **errp)
         close(monfd->fd);
         g_free(monfd->name);
         g_free(monfd);
+        qemu_mutex_unlock(&cur_mon->mon_lock);
         return;
     }
 
+    qemu_mutex_unlock(&cur_mon->mon_lock);
     error_setg(errp, QERR_FD_NOT_FOUND, fdname);
 }
 
@@ -2247,6 +2253,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;
 
@@ -2260,10 +2267,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.14.3

[Qemu-devel] [PATCH v4 3/4] monitor: more comments on lock-free fleids/funcs

[Peter Xu]

Add some explicit comment 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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index 9f361ec21e..6100ad44f8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -207,6 +207,7 @@ struct Monitor {
     int suspend_cnt;            /* Needs to be accessed atomically */
     bool skip_flush;
     bool use_io_thr;
+    /* Only used in parser, so no lock needed. */
     ReadLineState *rs;
     MonitorQMP qmp;
     gchar *mon_cpu_path;
@@ -1316,7 +1317,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.  BQL needed. */
 int monitor_set_cpu(int cpu_index)
 {
     CPUState *cpu;
@@ -1330,6 +1331,7 @@ int monitor_set_cpu(int cpu_index)
     return 0;
 }
 
+/* BQL neeeded. */
 static CPUState *mon_get_cpu_sync(bool synchronize)
 {
     CPUState *cpu;
-- 
2.14.3

[Qemu-devel] [PATCH v4 4/4] 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 | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6100ad44f8..c0c91cb70e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -263,6 +263,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;
@@ -279,6 +282,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?
  */
@@ -2306,9 +2319,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,
@@ -2343,6 +2358,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;
@@ -2362,10 +2378,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);
@@ -2381,6 +2399,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;
@@ -2410,6 +2429,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;
 }
@@ -2422,6 +2442,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 */
@@ -2442,6 +2463,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 */
@@ -2492,6 +2514,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;
 }
 
@@ -2501,7 +2524,9 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd;
     int mon_fd_flags;
+    int ret = -1;
 
+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
         if (mon_fdset->id != fdset_id) {
             continue;
@@ -2509,49 +2534,62 @@ 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;
+                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;
+        break;
     }
-#endif
-
+out:
+    qemu_mutex_unlock(&mon_fdsets_lock);
+    return ret;
+#else
     errno = ENOENT;
     return -1;
+#endif
 }
 
 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) {
@@ -2560,14 +2598,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 v4 2/4] monitor: protect mon->fds with mon_lock

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> mon->fds were protected by BQL.  Now protect it by mon_lock so that it
> can even be used in monitor iothread.
> 
> 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 48882d28ae..9f361ec21e 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.  */
> @@ -2207,6 +2207,7 @@ 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;
> @@ -2214,6 +2215,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>  
>          close(monfd->fd);
>          monfd->fd = fd;
> +        qemu_mutex_unlock(&cur_mon->mon_lock);

Why is it safe to have a close() in a mon_lock'd region?
We've got to make sure everything that happens in it is non-blocking.

Dave

>          return;
>      }
>  
> @@ -2222,12 +2224,14 @@ 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;
>  
> +    qemu_mutex_lock(&cur_mon->mon_lock);
>      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>          if (strcmp(monfd->name, fdname) != 0) {
>              continue;
> @@ -2237,9 +2241,11 @@ void qmp_closefd(const char *fdname, Error **errp)
>          close(monfd->fd);
>          g_free(monfd->name);
>          g_free(monfd);
> +        qemu_mutex_unlock(&cur_mon->mon_lock);
>          return;
>      }
>  
> +    qemu_mutex_unlock(&cur_mon->mon_lock);
>      error_setg(errp, QERR_FD_NOT_FOUND, fdname);
>  }
>  
> @@ -2247,6 +2253,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;
>  
> @@ -2260,10 +2267,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.14.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 2/4] monitor: protect mon->fds with mon_lock

[Peter Xu]

On Wed, May 02, 2018 at 12:15:07PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > mon->fds were protected by BQL.  Now protect it by mon_lock so that it
> > can even be used in monitor iothread.
> > 
> > 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 48882d28ae..9f361ec21e 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.  */
> > @@ -2207,6 +2207,7 @@ 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;
> > @@ -2214,6 +2215,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> >  
> >          close(monfd->fd);
> >          monfd->fd = fd;
> > +        qemu_mutex_unlock(&cur_mon->mon_lock);
> 
> Why is it safe to have a close() in a mon_lock'd region?
> We've got to make sure everything that happens in it is non-blocking.

Hmm indeed.  Let me move that close() out of the critical section. I
think I need to touch up below [1] too since it'll has similar
problem.

I'll wait for some more comments to repost.  Thanks,

> 
> Dave
> 
> >          return;
> >      }
> >  
> > @@ -2222,12 +2224,14 @@ 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;
> >  
> > +    qemu_mutex_lock(&cur_mon->mon_lock);
> >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> >          if (strcmp(monfd->name, fdname) != 0) {
> >              continue;
> > @@ -2237,9 +2241,11 @@ void qmp_closefd(const char *fdname, Error **errp)
> >          close(monfd->fd);
> >          g_free(monfd->name);
> >          g_free(monfd);
> > +        qemu_mutex_unlock(&cur_mon->mon_lock);

[1]

> >          return;
> >      }
> >  
> > +    qemu_mutex_unlock(&cur_mon->mon_lock);
> >      error_setg(errp, QERR_FD_NOT_FOUND, fdname);
> >  }
> >  
> > @@ -2247,6 +2253,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;
> >  
> > @@ -2260,10 +2267,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.14.3
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v4 3/4] monitor: more comments on lock-free fleids/funcs

[Stefan Hajnoczi]

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

On Wed, May 02, 2018 at 06:04:21PM +0800, Peter Xu wrote:
> Add some explicit comment 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 9f361ec21e..6100ad44f8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -207,6 +207,7 @@ struct Monitor {
>      int suspend_cnt;            /* Needs to be accessed atomically */
>      bool skip_flush;
>      bool use_io_thr;
> +    /* Only used in parser, so no lock needed. */
>      ReadLineState *rs;
>      MonitorQMP qmp;
>      gchar *mon_cpu_path;

Which fields does this comment cover?

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

Re: [Qemu-devel] [PATCH v4 1/4] monitor: rename out_lock to mon_lock

[Stefan Hajnoczi]

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

On Wed, May 02, 2018 at 06:04:19PM +0800, Peter Xu wrote:
> The out_lock was only protecting a few Monitor fields.  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.
> For now we share the lock.  We can split the lock when needed.
> 
> Since at it, arrange the Monitor struct a bit.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 53 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 24 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v4 4/4] monitor: add lock to protect mon_fdsets

[Stefan Hajnoczi]

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

On Wed, May 02, 2018 at 06:04:22PM +0800, Peter Xu wrote:
> 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 | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 53 insertions(+), 11 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v4 3/4] monitor: more comments on lock-free fleids/funcs

[Peter Xu]

On Tue, May 08, 2018 at 11:23:38AM +0100, Stefan Hajnoczi wrote:
> On Wed, May 02, 2018 at 06:04:21PM +0800, Peter Xu wrote:
> > Add some explicit comment 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 | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 9f361ec21e..6100ad44f8 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -207,6 +207,7 @@ struct Monitor {
> >      int suspend_cnt;            /* Needs to be accessed atomically */
> >      bool skip_flush;
> >      bool use_io_thr;
> > +    /* Only used in parser, so no lock needed. */
> >      ReadLineState *rs;
> >      MonitorQMP qmp;
> >      gchar *mon_cpu_path;
> 
> Which fields does this comment cover?

"rs" only.  Maybe I should move the comment to the end of line?

    ReadLineState *rs;       /* Only used in parser, so no lock needed. */

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v4 3/4] monitor: more comments on lock-free fleids/funcs

[Stefan Hajnoczi]

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

On Tue, May 08, 2018 at 06:32:32PM +0800, Peter Xu wrote:
> On Tue, May 08, 2018 at 11:23:38AM +0100, Stefan Hajnoczi wrote:
> > On Wed, May 02, 2018 at 06:04:21PM +0800, Peter Xu wrote:
> > > Add some explicit comment 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 | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/monitor.c b/monitor.c
> > > index 9f361ec21e..6100ad44f8 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -207,6 +207,7 @@ struct Monitor {
> > >      int suspend_cnt;            /* Needs to be accessed atomically */
> > >      bool skip_flush;
> > >      bool use_io_thr;
> > > +    /* Only used in parser, so no lock needed. */
> > >      ReadLineState *rs;
> > >      MonitorQMP qmp;
> > >      gchar *mon_cpu_path;
> > 
> > Which fields does this comment cover?
> 
> "rs" only.  Maybe I should move the comment to the end of line?
> 
>     ReadLineState *rs;       /* Only used in parser, so no lock needed. */

Yes, please.

Stefan

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