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

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

[Peter Xu]

v5:
- collect r-bs and rebase
- move two close()s outside critical section [Dave]
- move comment to end of line [Stefan]

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 | 144 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 103 insertions(+), 41 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v5 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.

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

[Qemu-devel] [PATCH v5 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 | 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 v5 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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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

[Qemu-devel] [PATCH v5 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.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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 ae5bca9d7c..d72b695156 100644
--- a/monitor.c
+++ b/monitor.c
@@ -262,6 +262,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;
@@ -278,6 +281,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?
  */
@@ -2307,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,
@@ -2344,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;
@@ -2363,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);
@@ -2382,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;
@@ -2411,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;
 }
@@ -2423,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 */
@@ -2443,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 */
@@ -2493,6 +2515,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;
 }
 
@@ -2502,7 +2525,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;
@@ -2510,49 +2535,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) {
@@ -2561,14 +2599,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.17.0

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

[Stefan Hajnoczi]

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

On Wed, May 09, 2018 at 12:17:32PM +0800, Peter Xu wrote:
> mon->fds were protected by BQL.  Now protect it by mon_lock so that it
> can even be used in monitor iothread.

Only monitor_get_fd() can safely be called from the monitor iothread
(oob).

The other functions call close(2), which may block, and therefore aren't
allowed in oob code.

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

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

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

[Stefan Hajnoczi]

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

On Wed, May 09, 2018 at 12:17:33PM +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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

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

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

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> The out_lock was only protecting a few Monitor fields.  In the future

"was protecting"?  When?  Or do you mean "is protecting"?

> 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 turn it into a bigger lock"?  If this patch does what its title
claims, it can't "turn" anything.  Do you perhaps mean "We're going to
turn it into a bigger lock"?

> For now we share the lock.  We can split the lock when needed.

What exactly do you mean by "we share the lock"?

> Since at it, arrange the Monitor struct a bit.

"Since we're at it, rearrange".  Can touch up on commit.

>
> 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;

This is an improvement.

>  };
>  
>  /* Let's add monitor global variables to this struct. */
[Remainder of patch snipped; it looks completely mechanical]

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

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

Appreciated!

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index d6c3c08932..ae5bca9d7c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -207,7 +207,7 @@ struct Monitor {
>      int suspend_cnt;            /* Needs to be accessed atomically */
>      bool skip_flush;
>      bool use_io_thr;
> -    ReadLineState *rs;
> +    ReadLineState *rs;   /* Only used in parser, so no lock needed. */

Pardon the ignorant question: why does "only used in parser" imply "no
lock needed"?

>      MonitorQMP qmp;
>      gchar *mon_cpu_path;
>      BlockCompletionFunc *password_completion_cb;
> @@ -1313,7 +1313,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. */

It's okay to start a comment containing a phrase with a lower case
letter, but you're turning this one into two sentences, and sentences
start in upper case.  Can touch up on commit.

"BQL needed" is okay, just a bit terse; I'd write "Caller must hold
BQL".  Could change that, too.

>  int monitor_set_cpu(int cpu_index)
>  {
>      CPUState *cpu;
> @@ -1327,6 +1327,7 @@ int monitor_set_cpu(int cpu_index)
>      return 0;
>  }
>  
> +/* BQL neeeded. */

Likewise.

>  static CPUState *mon_get_cpu_sync(bool synchronize)
>  {
>      CPUState *cpu;

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> 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>

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

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Similar to previous patch, but introduce a new global big lock for
> mon_fdsets.  Take it where needed.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 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 ae5bca9d7c..d72b695156 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -262,6 +262,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;
> @@ -278,6 +281,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?
>   */
> @@ -2307,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,
> @@ -2344,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;
> @@ -2363,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);
> @@ -2382,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;
> @@ -2411,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;
>  }
> @@ -2423,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 */
> @@ -2443,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 */
> @@ -2493,6 +2515,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;
>  }
>  
> @@ -2502,7 +2525,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;

Suggest not to initialize ret, and instead ret = -1 on both failure
paths.

>  
> +    qemu_mutex_lock(&mon_fdsets_lock);
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>          if (mon_fdset->id != fdset_id) {
>              continue;
> @@ -2510,49 +2535,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;

Preexisting: we fail without setting errno.  Smells buggy.

Can we avoid setting errno and return a negative errno code instead?

>              }
>  
>              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

#ifndef _WIN32 ... #endif becomes #ifndef _WIN32 ... #else ... #endif.
I hate negative conditionals with else.  Mind to swap?

>      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;

Dead initializer, please remove.

>  
> +    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;

Likewise.

>  
> +    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) {
> @@ -2561,14 +2599,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)

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

[Peter Xu]

On Thu, May 17, 2018 at 02:32:34PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > The out_lock was only protecting a few Monitor fields.  In the future
> 
> "was protecting"?  When?  Or do you mean "is protecting"?

Yes - "is protecting".

> 
> > 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 turn it into a bigger lock"?  If this patch does what its title
> claims, it can't "turn" anything.  Do you perhaps mean "We're going to
> turn it into a bigger lock"?

Yes - "are going to".

> 
> > For now we share the lock.  We can split the lock when needed.
> 
> What exactly do you mean by "we share the lock"?

I was trying to say that for now we will use a big lock for the whole
Monitor struct, then in the future we can split the big lock into
smaller ones when necessary.  It seems that it's actually bringing
misunderstanding instead of clearness.  So I think I will kill this
sentence directly.

>  > Since at it, arrange the Monitor struct a bit.  "Since we're at
> it, rearrange".  Can touch up on commit.

I guess I'll after all tune the commit message above and resend, I'll
just fix this too.

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

(though I'll keep the r-b for Stefan when repost; I hope it's fine)

> > 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;
> 
> This is an improvement.
> 
> >  };
> >  
> >  /* Let's add monitor global variables to this struct. */
> [Remainder of patch snipped; it looks completely mechanical]

Yes, it is.  Thanks,

-- 
Peter Xu

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

[Peter Xu]

On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Add some explicit comment for both Readline and cpu_set/cpu_get helpers
> > that they do not need the mon_lock protection.
> 
> Appreciated!
> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index d6c3c08932..ae5bca9d7c 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -207,7 +207,7 @@ struct Monitor {
> >      int suspend_cnt;            /* Needs to be accessed atomically */
> >      bool skip_flush;
> >      bool use_io_thr;
> > -    ReadLineState *rs;
> > +    ReadLineState *rs;   /* Only used in parser, so no lock needed. */
> 
> Pardon the ignorant question: why does "only used in parser" imply "no
> lock needed"?

Since even if the monitors can be run in multiple threads now, the
monitor parser of a specific Monitor will still only be run in either
the main thread or the monitor iothread.  My fault to be unclear on
the comment.  Maybe this one is better:

  It is only used in parser, and the parser of a monitor will only be
  run either in main thread or monitor IOThread but never both, so no
  lock is needed when accessing ReadLineState.

> 
> >      MonitorQMP qmp;
> >      gchar *mon_cpu_path;
> >      BlockCompletionFunc *password_completion_cb;
> > @@ -1313,7 +1313,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. */
> 
> It's okay to start a comment containing a phrase with a lower case
> letter, but you're turning this one into two sentences, and sentences
> start in upper case.  Can touch up on commit.
> 
> "BQL needed" is okay, just a bit terse; I'd write "Caller must hold
> BQL".  Could change that, too.

I'll do that.

> 
> >  int monitor_set_cpu(int cpu_index)
> >  {
> >      CPUState *cpu;
> > @@ -1327,6 +1327,7 @@ int monitor_set_cpu(int cpu_index)
> >      return 0;
> >  }
> >  
> > +/* BQL neeeded. */
> 
> Likewise.

Will do.  Thanks,

-- 
Peter Xu

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

[Peter Xu]

On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:

[...]

> > @@ -2502,7 +2525,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;
> 
> Suggest not to initialize ret, and instead ret = -1 on both failure
> paths.

[1]

But there is a third hidden failure path that we failed to find the fd
specified?  In that case we still need that initial value.

But I didn't really notice that this function is returning error with
-1 paired with errno.  So instead of set -1 here I may need to
initialize it to -ENOENT, and I can convert it back to errno when
return.  Please see below.

> 
> >  
> > +    qemu_mutex_lock(&mon_fdsets_lock);
> >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> >          if (mon_fdset->id != fdset_id) {
> >              continue;
> > @@ -2510,49 +2535,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;
> 
> Preexisting: we fail without setting errno.  Smells buggy.

Indeed.  Here I possibly need to set "ret = -errno" since at [2] below
the errno might be polluted by the mutex unlocking operation.

> 
> Can we avoid setting errno and return a negative errno code instead?

Yes that'll be nice, but it's getting out of the scope of this
patchset.  So I'll try to avoid touching that.  I mean qemu_open() and
its callers.

> 
> >              }
> >  
> >              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);

[2]

> > +    return ret;

So in my next post I'll make sure I return -1 when error happens, and
errno should contain the correct (positive) error.

> > +#else
> 
> #ifndef _WIN32 ... #endif becomes #ifndef _WIN32 ... #else ... #endif.
> I hate negative conditionals with else.  Mind to swap?

Sure I can.

> 
> >      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;
> 
> Dead initializer, please remove.

IMHO it's the same as above [1], so we still need that, right?

> 
> >  
> > +    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;
> 
> Likewise.

Same as [1]?

Thanks,

-- 
Peter Xu

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:
>
> [...]
>
>> > @@ -2502,7 +2525,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;
>> 
>> Suggest not to initialize ret, and instead ret = -1 on both failure
>> paths.
>
> [1]
>
> But there is a third hidden failure path that we failed to find the fd
> specified?  In that case we still need that initial value.

You're right.  However, that failure path could be made explicit easily:

        QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
            [got out on error and on finding the right one...]
        }
        ret = -1;
        errno = ENOENT;

    out:
        qemu_mutex_unlock(&mon_fdsets_lock);
        return ret;

I find this clearer.  Your choice.

> But I didn't really notice that this function is returning error with
> -1 paired with errno.  So instead of set -1 here I may need to
> initialize it to -ENOENT, and I can convert it back to errno when
> return.  Please see below.
>
>> 
>> >  
>> > +    qemu_mutex_lock(&mon_fdsets_lock);
>> >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> >          if (mon_fdset->id != fdset_id) {
>> >              continue;
>> > @@ -2510,49 +2535,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;
>> 
>> Preexisting: we fail without setting errno.  Smells buggy.
>
> Indeed.  Here I possibly need to set "ret = -errno" since at [2] below
> the errno might be polluted by the mutex unlocking operation.

Good point.

>> Can we avoid setting errno and return a negative errno code instead?
>
> Yes that'll be nice, but it's getting out of the scope of this
> patchset.  So I'll try to avoid touching that.  I mean qemu_open() and
> its callers.

I'd change just monitor_fdset_get_fd(), and have its only caller
qemu_open() do

        fd = monitor_fdset_get_fd(fdset_id, flags);
        if (fd < 0) {
            errno = -fd;
            return -1;
        }

>> >              }
>> >  
>> >              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);
>
> [2]
>
>> > +    return ret;
>
> So in my next post I'll make sure I return -1 when error happens, and
> errno should contain the correct (positive) error.
>
>> > +#else
>> 
>> #ifndef _WIN32 ... #endif becomes #ifndef _WIN32 ... #else ... #endif.
>> I hate negative conditionals with else.  Mind to swap?
>
> Sure I can.
>
>> 
>> >      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;
>> 
>> Dead initializer, please remove.
>
> IMHO it's the same as above [1], so we still need that, right?

You're right.

>> >  
>> > +    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;
>> 
>> Likewise.
>
> Same as [1]?

Right again.

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

[Peter Xu]

On Fri, May 18, 2018 at 02:27:00PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:
> >
> > [...]
> >
> >> > @@ -2502,7 +2525,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;
> >> 
> >> Suggest not to initialize ret, and instead ret = -1 on both failure
> >> paths.
> >
> > [1]
> >
> > But there is a third hidden failure path that we failed to find the fd
> > specified?  In that case we still need that initial value.
> 
> You're right.  However, that failure path could be made explicit easily:
> 
>         QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>             [got out on error and on finding the right one...]
>         }
>         ret = -1;
>         errno = ENOENT;
> 
>     out:
>         qemu_mutex_unlock(&mon_fdsets_lock);
>         return ret;
> 
> I find this clearer.  Your choice.

Yes this works too.  Considering that I just posted v6, I'll
temporarily just keep the old way.

> 
> > But I didn't really notice that this function is returning error with
> > -1 paired with errno.  So instead of set -1 here I may need to
> > initialize it to -ENOENT, and I can convert it back to errno when
> > return.  Please see below.
> >
> >> 
> >> >  
> >> > +    qemu_mutex_lock(&mon_fdsets_lock);
> >> >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> >> >          if (mon_fdset->id != fdset_id) {
> >> >              continue;
> >> > @@ -2510,49 +2535,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;
> >> 
> >> Preexisting: we fail without setting errno.  Smells buggy.
> >
> > Indeed.  Here I possibly need to set "ret = -errno" since at [2] below
> > the errno might be polluted by the mutex unlocking operation.
> 
> Good point.
> 
> >> Can we avoid setting errno and return a negative errno code instead?
> >
> > Yes that'll be nice, but it's getting out of the scope of this
> > patchset.  So I'll try to avoid touching that.  I mean qemu_open() and
> > its callers.
> 
> I'd change just monitor_fdset_get_fd(), and have its only caller
> qemu_open() do
> 
>         fd = monitor_fdset_get_fd(fdset_id, flags);
>         if (fd < 0) {
>             errno = -fd;
>             return -1;
>         }

Yes this I can do.  I'll avoid resending for this change only (and
IMHO it can also be a follow-up patch).  If the latest version 6 will
need further refinings I'll touch up qemu_open() for this altogether.

Thanks,

-- 
Peter Xu

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Add some explicit comment for both Readline and cpu_set/cpu_get helpers
>> > that they do not need the mon_lock protection.
>> 
>> Appreciated!
>> 
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  monitor.c | 5 +++--
>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index d6c3c08932..ae5bca9d7c 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -207,7 +207,7 @@ struct Monitor {
>> >      int suspend_cnt;            /* Needs to be accessed atomically */
>> >      bool skip_flush;
>> >      bool use_io_thr;
>> > -    ReadLineState *rs;
>> > +    ReadLineState *rs;   /* Only used in parser, so no lock needed. */
>> 
>> Pardon the ignorant question: why does "only used in parser" imply "no
>> lock needed"?
>
> Since even if the monitors can be run in multiple threads now, the
> monitor parser of a specific Monitor will still only be run in either
> the main thread or the monitor iothread.  My fault to be unclear on
> the comment.  Maybe this one is better:
>
>   It is only used in parser, and the parser of a monitor will only be
>   run either in main thread or monitor IOThread but never both, so no
>   lock is needed when accessing ReadLineState.

One further question, just to help me understand how this stuff works:
what are the conditions for the parser running in the main thread, and
what are the conditions for it running in the monitor IOThread?

[...]

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Fri, May 18, 2018 at 02:27:00PM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:
>> >
>> > [...]
>> >
>> >> > @@ -2502,7 +2525,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;
>> >> 
>> >> Suggest not to initialize ret, and instead ret = -1 on both failure
>> >> paths.
>> >
>> > [1]
>> >
>> > But there is a third hidden failure path that we failed to find the fd
>> > specified?  In that case we still need that initial value.
>> 
>> You're right.  However, that failure path could be made explicit easily:
>> 
>>         QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>>             [got out on error and on finding the right one...]
>>         }
>>         ret = -1;
>>         errno = ENOENT;
>> 
>>     out:
>>         qemu_mutex_unlock(&mon_fdsets_lock);
>>         return ret;
>> 
>> I find this clearer.  Your choice.
>
> Yes this works too.  Considering that I just posted v6, I'll
> temporarily just keep the old way.

Your v6 raced with my review of v5.  Do you intend to post v7?  If not,
I need to figure out what I can and want to do to v6 on commit to my
tree.

>> > But I didn't really notice that this function is returning error with
>> > -1 paired with errno.  So instead of set -1 here I may need to
>> > initialize it to -ENOENT, and I can convert it back to errno when
>> > return.  Please see below.
>> >
>> >> 
>> >> >  
>> >> > +    qemu_mutex_lock(&mon_fdsets_lock);
>> >> >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> >> >          if (mon_fdset->id != fdset_id) {
>> >> >              continue;
>> >> > @@ -2510,49 +2535,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;
>> >> 
>> >> Preexisting: we fail without setting errno.  Smells buggy.
>> >
>> > Indeed.  Here I possibly need to set "ret = -errno" since at [2] below
>> > the errno might be polluted by the mutex unlocking operation.
>> 
>> Good point.
>> 
>> >> Can we avoid setting errno and return a negative errno code instead?
>> >
>> > Yes that'll be nice, but it's getting out of the scope of this
>> > patchset.  So I'll try to avoid touching that.  I mean qemu_open() and
>> > its callers.
>> 
>> I'd change just monitor_fdset_get_fd(), and have its only caller
>> qemu_open() do
>> 
>>         fd = monitor_fdset_get_fd(fdset_id, flags);
>>         if (fd < 0) {
>>             errno = -fd;
>>             return -1;
>>         }
>
> Yes this I can do.  I'll avoid resending for this change only (and
> IMHO it can also be a follow-up patch).

Followup patch is fine.

>                                          If the latest version 6 will
> need further refinings I'll touch up qemu_open() for this altogether.

Just to avoid misunderstandings: I'm not asking you to change
qemu_open()'s contract.  Since qemu_open() is basically a compatibility
helper to emulate modern open() with O_CLOEXEC on old systems, with some
entirely undocumented fd set functionality thrown in (grrr...), having
it set errno on failure just like open() makes some sense.

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

[Peter Xu]

On Wed, May 23, 2018 at 10:29:37AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > Add some explicit comment for both Readline and cpu_set/cpu_get helpers
> >> > that they do not need the mon_lock protection.
> >> 
> >> Appreciated!
> >> 
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> > ---
> >> >  monitor.c | 5 +++--
> >> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/monitor.c b/monitor.c
> >> > index d6c3c08932..ae5bca9d7c 100644
> >> > --- a/monitor.c
> >> > +++ b/monitor.c
> >> > @@ -207,7 +207,7 @@ struct Monitor {
> >> >      int suspend_cnt;            /* Needs to be accessed atomically */
> >> >      bool skip_flush;
> >> >      bool use_io_thr;
> >> > -    ReadLineState *rs;
> >> > +    ReadLineState *rs;   /* Only used in parser, so no lock needed. */
> >> 
> >> Pardon the ignorant question: why does "only used in parser" imply "no
> >> lock needed"?
> >
> > Since even if the monitors can be run in multiple threads now, the
> > monitor parser of a specific Monitor will still only be run in either
> > the main thread or the monitor iothread.  My fault to be unclear on
> > the comment.  Maybe this one is better:
> >
> >   It is only used in parser, and the parser of a monitor will only be
> >   run either in main thread or monitor IOThread but never both, so no
> >   lock is needed when accessing ReadLineState.
> 
> One further question, just to help me understand how this stuff works:
> what are the conditions for the parser running in the main thread, and
> what are the conditions for it running in the monitor IOThread?

For QMP parsers, the place is decided by Monitor.use_io_thr.  If set,
the parser runs in monitor IOThread; otherwise it still runs in main
thread.

For HMP parsers, they should always been run in the main thread.

After replying I just noticed that ReadLineState should only be used
by HMP, or to be more explicit, when MONITOR_USE_READLINE is set.  So
maybe the comment is not really accurate above - actually it never
runs in monitor iothread!  However the conclusion is still the same -
we don't need to protect it.

Thanks,

-- 
Peter Xu

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

[Peter Xu]

On Wed, May 23, 2018 at 10:39:20AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Fri, May 18, 2018 at 02:27:00PM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:
> >> >
> >> > [...]
> >> >
> >> >> > @@ -2502,7 +2525,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;
> >> >> 
> >> >> Suggest not to initialize ret, and instead ret = -1 on both failure
> >> >> paths.
> >> >
> >> > [1]
> >> >
> >> > But there is a third hidden failure path that we failed to find the fd
> >> > specified?  In that case we still need that initial value.
> >> 
> >> You're right.  However, that failure path could be made explicit easily:
> >> 
> >>         QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> >>             [got out on error and on finding the right one...]
> >>         }
> >>         ret = -1;
> >>         errno = ENOENT;
> >> 
> >>     out:
> >>         qemu_mutex_unlock(&mon_fdsets_lock);
> >>         return ret;
> >> 
> >> I find this clearer.  Your choice.
> >
> > Yes this works too.  Considering that I just posted v6, I'll
> > temporarily just keep the old way.
> 
> Your v6 raced with my review of v5.  Do you intend to post v7?  If not,
> I need to figure out what I can and want to do to v6 on commit to my
> tree.

I can repost v7 after we finish the discussion in the other thread:

  [PATCH v5 3/4] monitor: more comments on lock-free fleids/funcs
  Message-ID: <87muwqixla.fsf@dusky.pond.sub.org>

I think there is a comment to be refined, meanwhile I can at least
pick up the qemu_open() suggestion too.

Regards,

-- 
Peter Xu

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, May 23, 2018 at 10:29:37AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > Add some explicit comment for both Readline and cpu_set/cpu_get helpers
>> >> > that they do not need the mon_lock protection.
>> >> 
>> >> Appreciated!
>> >> 
>> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> >> > ---
>> >> >  monitor.c | 5 +++--
>> >> >  1 file changed, 3 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/monitor.c b/monitor.c
>> >> > index d6c3c08932..ae5bca9d7c 100644
>> >> > --- a/monitor.c
>> >> > +++ b/monitor.c
>> >> > @@ -207,7 +207,7 @@ struct Monitor {
>> >> >      int suspend_cnt;            /* Needs to be accessed atomically */
>> >> >      bool skip_flush;
>> >> >      bool use_io_thr;
>> >> > -    ReadLineState *rs;
>> >> > +    ReadLineState *rs;   /* Only used in parser, so no lock needed. */
>> >> 
>> >> Pardon the ignorant question: why does "only used in parser" imply "no
>> >> lock needed"?
>> >
>> > Since even if the monitors can be run in multiple threads now, the
>> > monitor parser of a specific Monitor will still only be run in either
>> > the main thread or the monitor iothread.  My fault to be unclear on
>> > the comment.  Maybe this one is better:
>> >
>> >   It is only used in parser, and the parser of a monitor will only be
>> >   run either in main thread or monitor IOThread but never both, so no
>> >   lock is needed when accessing ReadLineState.
>> 
>> One further question, just to help me understand how this stuff works:
>> what are the conditions for the parser running in the main thread, and
>> what are the conditions for it running in the monitor IOThread?
>
> For QMP parsers, the place is decided by Monitor.use_io_thr.  If set,

Aside: spelling it use_io_thread would buy us a bit of readability at a
total cost of some 30 characters :)

> the parser runs in monitor IOThread; otherwise it still runs in main
> thread.

Commit a5ed352596a and 3fd2457d18e.  I see.

> For HMP parsers, they should always been run in the main thread.
>
> After replying I just noticed that ReadLineState should only be used
> by HMP, or to be more explicit, when MONITOR_USE_READLINE is set.  So
> maybe the comment is not really accurate above - actually it never
> runs in monitor iothread!  However the conclusion is still the same -
> we don't need to protect it.

Flags MONITOR_USE_READLINE and MONITOR_USE_CONTROL are independent.
However, our CLI currently supports mode=readline (MONITOR_USE_READLINE)
and mode=control (MONITOR_USE_CONTROL).

If relying on "MONITOR_USE_CONTROL implies !MONITOR_USE_READLINE" makes
things simpler, no objections from me, but we should add an assertion.

Back to the comment on member @rs.  What about

    /*
     * 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;
    // other members that aren't shared, if any

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

[Peter Xu]

On Wed, May 23, 2018 at 05:15:26PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, May 23, 2018 at 10:29:37AM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote:
> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >> 
> >> >> > Add some explicit comment for both Readline and cpu_set/cpu_get helpers
> >> >> > that they do not need the mon_lock protection.
> >> >> 
> >> >> Appreciated!
> >> >> 
> >> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> >> > ---
> >> >> >  monitor.c | 5 +++--
> >> >> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >> >> >
> >> >> > diff --git a/monitor.c b/monitor.c
> >> >> > index d6c3c08932..ae5bca9d7c 100644
> >> >> > --- a/monitor.c
> >> >> > +++ b/monitor.c
> >> >> > @@ -207,7 +207,7 @@ struct Monitor {
> >> >> >      int suspend_cnt;            /* Needs to be accessed atomically */
> >> >> >      bool skip_flush;
> >> >> >      bool use_io_thr;
> >> >> > -    ReadLineState *rs;
> >> >> > +    ReadLineState *rs;   /* Only used in parser, so no lock needed. */
> >> >> 
> >> >> Pardon the ignorant question: why does "only used in parser" imply "no
> >> >> lock needed"?
> >> >
> >> > Since even if the monitors can be run in multiple threads now, the
> >> > monitor parser of a specific Monitor will still only be run in either
> >> > the main thread or the monitor iothread.  My fault to be unclear on
> >> > the comment.  Maybe this one is better:
> >> >
> >> >   It is only used in parser, and the parser of a monitor will only be
> >> >   run either in main thread or monitor IOThread but never both, so no
> >> >   lock is needed when accessing ReadLineState.
> >> 
> >> One further question, just to help me understand how this stuff works:
> >> what are the conditions for the parser running in the main thread, and
> >> what are the conditions for it running in the monitor IOThread?
> >
> > For QMP parsers, the place is decided by Monitor.use_io_thr.  If set,
> 
> Aside: spelling it use_io_thread would buy us a bit of readability at a
> total cost of some 30 characters :)

Sorry for the bad names...

Please feel free to change that as follow up patches on any of the
namings.  I am never good at that. :(

> 
> > the parser runs in monitor IOThread; otherwise it still runs in main
> > thread.
> 
> Commit a5ed352596a and 3fd2457d18e.  I see.
> 
> > For HMP parsers, they should always been run in the main thread.
> >
> > After replying I just noticed that ReadLineState should only be used
> > by HMP, or to be more explicit, when MONITOR_USE_READLINE is set.  So
> > maybe the comment is not really accurate above - actually it never
> > runs in monitor iothread!  However the conclusion is still the same -
> > we don't need to protect it.
> 
> Flags MONITOR_USE_READLINE and MONITOR_USE_CONTROL are independent.
> However, our CLI currently supports mode=readline (MONITOR_USE_READLINE)
> and mode=control (MONITOR_USE_CONTROL).

Yeah, so it seems to me the truth is that they are dependent no matter
how we implemented the flags.

> 
> If relying on "MONITOR_USE_CONTROL implies !MONITOR_USE_READLINE" makes
> things simpler, no objections from me, but we should add an assertion.
> 
> Back to the comment on member @rs.  What about
> 
>     /*
>      * 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;
>     // other members that aren't shared, if any

Sure!  Thanks for offering.

-- 
Peter Xu