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

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

[Peter Xu]

v7:
- let monitor_fdset_get_fd() return -errno, touch up qemu_open() to
  translate that into errno.  [Markus]
- touch up comment for Monitor.rs
- rebase, and torture the code a bit with qtest.

v6:
- add/drop some r-bs
- address all the comments from Markus
- rebase, and run some simple qtests to make sure nothing breaks

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    | 155 +++++++++++++++++++++++++++++++++++++--------------
 util/osdep.c |   3 +-
 2 files changed, 115 insertions(+), 43 deletions(-)

-- 
2.17.0

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

[Peter Xu]

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

Since at it, rearrange the Monitor struct a bit.

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

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

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

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

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

[Qemu-devel] [PATCH v7 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 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

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

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

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

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c    | 70 +++++++++++++++++++++++++++++++++++++++++-----------
 util/osdep.c |  3 ++-
 2 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/monitor.c b/monitor.c
index f01589f945..6266ff65c4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -271,6 +271,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;
@@ -287,6 +290,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?
  */
@@ -2316,9 +2329,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,
@@ -2353,6 +2368,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;
@@ -2372,10 +2388,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);
@@ -2391,6 +2409,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;
@@ -2420,6 +2439,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;
 }
@@ -2432,6 +2452,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 */
@@ -2452,6 +2473,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 */
@@ -2502,16 +2524,21 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
     fdinfo->fdset_id = mon_fdset->id;
     fdinfo->fd = mon_fdset_fd->fd;
 
+    qemu_mutex_unlock(&mon_fdsets_lock);
     return fdinfo;
 }
 
 int monitor_fdset_get_fd(int64_t fdset_id, int flags)
 {
-#ifndef _WIN32
+#ifdef _WIN32
+    return -ENOENT;
+#else
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd;
     int mon_fd_flags;
+    int ret = -ENOENT;
 
+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
         if (mon_fdset->id != fdset_id) {
             continue;
@@ -2519,49 +2546,60 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
         QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
             mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
             if (mon_fd_flags == -1) {
-                return -1;
+                ret = -errno;
+                goto out;
             }
 
             if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
-                return mon_fdset_fd->fd;
+                ret = mon_fdset_fd->fd;
+                goto out;
             }
         }
-        errno = EACCES;
-        return -1;
+        ret = -EACCES;
+        break;
     }
+out:
+    qemu_mutex_unlock(&mon_fdsets_lock);
+    return ret;
 #endif
-
-    errno = ENOENT;
-    return -1;
 }
 
 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) {
@@ -2570,14 +2608,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)
diff --git a/util/osdep.c b/util/osdep.c
index a73de0e1ba..ea51d500b6 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -302,7 +302,8 @@ int qemu_open(const char *name, int flags, ...)
         }
 
         fd = monitor_fdset_get_fd(fdset_id, flags);
-        if (fd == -1) {
+        if (fd < 0) {
+            errno = -fd;
             return -1;
         }
 
-- 
2.17.0

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180524043952.11245-1-peterx@redhat.com
Subject: [Qemu-devel] [PATCH v7 0/4] monitor: let Monitor be thread safe

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20180522035629.30428-1-peterx@redhat.com -> patchew/20180522035629.30428-1-peterx@redhat.com
 * [new tag]               patchew/20180524043952.11245-1-peterx@redhat.com -> patchew/20180524043952.11245-1-peterx@redhat.com
Switched to a new branch 'test'
dadcba2375 monitor: add lock to protect mon_fdsets
eda66671b9 monitor: more comments on lock-free fleids/funcs
b7d356e160 monitor: protect mon->fds with mon_lock
35eadfaff2 monitor: rename out_lock to mon_lock

=== OUTPUT BEGIN ===
Checking PATCH 1/4: monitor: rename out_lock to mon_lock...
Checking PATCH 2/4: monitor: protect mon->fds with mon_lock...
Checking PATCH 3/4: monitor: more comments on lock-free fleids/funcs...
ERROR: do not use C99 // comments
#28: FILE: monitor.c:218:
+    // other members that aren't shared

total: 1 errors, 0 warnings, 31 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/4: monitor: add lock to protect mon_fdsets...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Stefan Hajnoczi]

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

On Thu, May 24, 2018 at 12:39:51PM +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 | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)

Aside from the checkpatch.pl error that needs to be fixed (I like to use
a git hook to check automatically, see
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html):

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

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

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

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

Hmm, not sure about "all the rest": PATCH 3 marks a member as not
needing protection.  Shall I change this to "most of the rest" when I
apply?

>
> Since at it, rearrange the Monitor struct a bit.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

[Markus Armbruster]

Regarding the subject: what are "fleids"?

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.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/monitor.c b/monitor.c
> index d6c3c08932..f01589f945 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -207,7 +207,16 @@ struct Monitor {
>      int suspend_cnt;            /* Needs to be accessed atomically */
>      bool skip_flush;
>      bool use_io_thr;
> +
> +    /*
> +     * State used only in the thread "owning" the monitor.
> +     * If @use_io_thr, this is mon_global.mon_iothread.
> +     * Else, it's the main thread.
> +     * These members can be safely accessed without locks.
> +     */
>      ReadLineState *rs;
> +    // other members that aren't shared

Whoops, misunderstanding!  I meant this line as a placeholder, to
further illustrate my intent.  It should not be committed.  If we need a
comment here, it should use /* traditional comment syntax */.

> +
>      MonitorQMP qmp;
>      gchar *mon_cpu_path;
>      BlockCompletionFunc *password_completion_cb;
> @@ -1313,7 +1322,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
>      cur_mon->qmp.commands = &qmp_commands;
>  }
>  
> -/* set the current CPU defined by the user */
> +/* Set the current CPU defined by the user. Callers must hold BQL. */
>  int monitor_set_cpu(int cpu_index)
>  {
>      CPUState *cpu;
> @@ -1327,6 +1336,7 @@ int monitor_set_cpu(int cpu_index)
>      return 0;
>  }
>  
> +/* Callers must hold BQL. */
>  static CPUState *mon_get_cpu_sync(bool synchronize)
>  {
>      CPUState *cpu;

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

[Peter Xu]

On Thu, May 24, 2018 at 10:29:20AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > The out_lock is protecting a few Monitor fields.  In the future the
> > monitor code will start to run in multiple threads.  We are going to
> > turn it into a bigger lock to protect not only the out buffer but also
> > all the rest.
> 
> Hmm, not sure about "all the rest": PATCH 3 marks a member as not
> needing protection.  Shall I change this to "most of the rest" when I
> apply?

Of course. :) Please feel free to touch up the commit message where
necessary (this rule applies to all my patches).  I fully trust your
wordings.

Thanks!

-- 
Peter Xu

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

[Peter Xu]

On Thu, May 24, 2018 at 10:41:09AM +0200, Markus Armbruster wrote:
> Regarding the subject: what are "fleids"?

Ouch. :( I meant the word "fields".

> 
> 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.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index d6c3c08932..f01589f945 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -207,7 +207,16 @@ struct Monitor {
> >      int suspend_cnt;            /* Needs to be accessed atomically */
> >      bool skip_flush;
> >      bool use_io_thr;
> > +
> > +    /*
> > +     * State used only in the thread "owning" the monitor.
> > +     * If @use_io_thr, this is mon_global.mon_iothread.
> > +     * Else, it's the main thread.
> > +     * These members can be safely accessed without locks.
> > +     */
> >      ReadLineState *rs;
> > +    // other members that aren't shared
> 
> Whoops, misunderstanding!  I meant this line as a placeholder, to
> further illustrate my intent.  It should not be committed.  If we need a
> comment here, it should use /* traditional comment syntax */.

My fault.

Please let me know if you want me to repost...  Thanks,

-- 
Peter Xu

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

[Peter Xu]

On Thu, May 24, 2018 at 09:08:04AM +0100, Stefan Hajnoczi wrote:
> On Thu, May 24, 2018 at 12:39:51PM +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 | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> Aside from the checkpatch.pl error that needs to be fixed (I like to use
> a git hook to check automatically, see
> http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html):
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks Stefan.  I start to use it now!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v7 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.

The previous patch is "monitor: more comments on lock-free
fleids/funcs".  Sure you mean that one?
>
> The monitor_fdset_get_fd() handling is a bit tricky: now we need to call
> qemu_mutex_unlock() which might pollute errno, so we need to make sure
> the correct errno be passed up to the callers.  To make things simpler,
> we let monitor_fdset_get_fd() return the -errno directly when error
> happens, then in qemu_open() we translate it back into errno.

Suggest s/translate/move/.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c    | 70 +++++++++++++++++++++++++++++++++++++++++-----------
>  util/osdep.c |  3 ++-
>  2 files changed, 58 insertions(+), 15 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index f01589f945..6266ff65c4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -271,6 +271,9 @@ typedef struct QMPRequest QMPRequest;
>  /* Protects mon_list, monitor_event_state.  */

Not this patch's problem: there is no monitor_event_state.  Screwed up
in commit d622cb5879c.  I guess it's monitor_qapi_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;

Suggest to move mon_fdsets next to the lock protecting it.

> @@ -287,6 +290,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.

Do you mean CLI parsing?

> + * Meanwhile it can also be used even at the end of main.  Let's keep
> + * it initialized for the whole lifecycle of QEMU.
> + */

Awkward question, since our main() is such a tangled mess, but here goes
anyway...  The existing place to initialize monitor.c's globals is
monitor_init_globals().  But that one runs too late, I guess:
parse_add_fd() runs earlier, and calls monitor_fdset_add_fd().  Unclean
even without this lock; no module should be used before its
initialization function runs.  Sure we can't run monitor_init_globals()
sufficiently early?

> +static void __attribute__((constructor)) mon_fdsets_lock_init(void)
> +{
> +    qemu_mutex_init(&mon_fdsets_lock);
> +}
> +
>  /**
>   * Is @mon a QMP monitor?
>   */
> @@ -2316,9 +2329,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,
> @@ -2353,6 +2368,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;
> @@ -2372,10 +2388,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);
> @@ -2391,6 +2409,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;
> @@ -2420,6 +2439,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;
>  }
> @@ -2432,6 +2452,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 */
> @@ -2452,6 +2473,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 */
> @@ -2502,16 +2524,21 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>      fdinfo->fdset_id = mon_fdset->id;
>      fdinfo->fd = mon_fdset_fd->fd;
>  
> +    qemu_mutex_unlock(&mon_fdsets_lock);
>      return fdinfo;
>  }
>  
>  int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>  {
> -#ifndef _WIN32
> +#ifdef _WIN32
> +    return -ENOENT;

ENOENT feels odd, but I guess makes some sense since there is no way to
add entries.

> +#else
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd;
>      int mon_fd_flags;
> +    int ret = -ENOENT;
>  
> +    qemu_mutex_lock(&mon_fdsets_lock);
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>          if (mon_fdset->id != fdset_id) {
>              continue;
> @@ -2519,49 +2546,60 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>          QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
>              mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
>              if (mon_fd_flags == -1) {
> -                return -1;
> +                ret = -errno;
> +                goto out;
>              }
>  
>              if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
> -                return mon_fdset_fd->fd;
> +                ret = mon_fdset_fd->fd;
> +                goto out;
>              }
>          }
> -        errno = EACCES;
> -        return -1;
> +        ret = -EACCES;
> +        break;
>      }

I still think

           ret = -EACCES;
           goto out;
       }
       ret = -ENOENT;

   out:

would be clearer.

> +out:
> +    qemu_mutex_unlock(&mon_fdsets_lock);
> +    return ret;
>  #endif
> -
> -    errno = ENOENT;
> -    return -1;
>  }
>  
>  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;

Dead assignment.  Alternatively,

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

... add

       ret = -1;

here, and drop the initializer.  Your choice.

> -    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) {
> @@ -2570,14 +2608,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;
>  }

Likewise.

>  
>  int monitor_fdset_dup_fd_find(int dup_fd)
> diff --git a/util/osdep.c b/util/osdep.c
> index a73de0e1ba..ea51d500b6 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -302,7 +302,8 @@ int qemu_open(const char *name, int flags, ...)
>          }
>  
>          fd = monitor_fdset_get_fd(fdset_id, flags);
> -        if (fd == -1) {
> +        if (fd < 0) {
> +            errno = -fd;
>              return -1;
>          }

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

[Stefan Hajnoczi]

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

On Thu, May 24, 2018 at 12:39:52PM +0800, Peter Xu wrote:
>  int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>  {
> -#ifndef _WIN32
> +#ifdef _WIN32
> +    return -ENOENT;

stubs/fdset.c:monitor_fdset_get_fd() should return -ENOENT instead of -1
now.

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

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 24, 2018 at 10:29:20AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > The out_lock is protecting a few Monitor fields.  In the future the
>> > monitor code will start to run in multiple threads.  We are going to
>> > turn it into a bigger lock to protect not only the out buffer but also
>> > all the rest.
>> 
>> Hmm, not sure about "all the rest": PATCH 3 marks a member as not
>> needing protection.  Shall I change this to "most of the rest" when I
>> apply?
>
> Of course. :) Please feel free to touch up the commit message where
> necessary (this rule applies to all my patches).  I fully trust your
> wordings.

Well, I don't, so I'll keep asking :)

> Thanks!

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 24, 2018 at 10:41:09AM +0200, Markus Armbruster wrote:
>> Regarding the subject: what are "fleids"?
>
> Ouch. :( I meant the word "fields".

Can touch that up in my tree.

>> 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.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  monitor.c | 12 +++++++++++-
>> >  1 file changed, 11 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index d6c3c08932..f01589f945 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -207,7 +207,16 @@ struct Monitor {
>> >      int suspend_cnt;            /* Needs to be accessed atomically */
>> >      bool skip_flush;
>> >      bool use_io_thr;
>> > +
>> > +    /*
>> > +     * State used only in the thread "owning" the monitor.
>> > +     * If @use_io_thr, this is mon_global.mon_iothread.
>> > +     * Else, it's the main thread.
>> > +     * These members can be safely accessed without locks.
>> > +     */
>> >      ReadLineState *rs;
>> > +    // other members that aren't shared
>> 
>> Whoops, misunderstanding!  I meant this line as a placeholder, to
>> further illustrate my intent.  It should not be committed.  If we need a
>> comment here, it should use /* traditional comment syntax */.
>
> My fault.
>
> Please let me know if you want me to repost...  Thanks,

Can touch that up, too.  No respin needed unless something more complex
comes up.

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

[Peter Xu]

On Thu, May 24, 2018 at 10:28:37AM +0100, Stefan Hajnoczi wrote:
> On Thu, May 24, 2018 at 12:39:52PM +0800, Peter Xu wrote:
> >  int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> >  {
> > -#ifndef _WIN32
> > +#ifdef _WIN32
> > +    return -ENOENT;
> 
> stubs/fdset.c:monitor_fdset_get_fd() should return -ENOENT instead of -1
> now.

Yes that's intended.  That's actually a suggestion from Markus since
changing the return code will simplify the code.  I mentioned it in
the commit message too:

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

Thanks,

-- 
Peter Xu

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

[Stefan Hajnoczi]

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

On Fri, May 25, 2018 at 11:30:22AM +0800, Peter Xu wrote:
> On Thu, May 24, 2018 at 10:28:37AM +0100, Stefan Hajnoczi wrote:
> > On Thu, May 24, 2018 at 12:39:52PM +0800, Peter Xu wrote:
> > >  int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> > >  {
> > > -#ifndef _WIN32
> > > +#ifdef _WIN32
> > > +    return -ENOENT;
> > 
> > stubs/fdset.c:monitor_fdset_get_fd() should return -ENOENT instead of -1
> > now.
> 
> Yes that's intended.  That's actually a suggestion from Markus since
> changing the return code will simplify the code.

No, I understand that part.  I'm pointing out that
stubs/fdset.c:monitor_fdset_get_fd() still returns -1 because it was not
updated by this patch.

Since this patch changes the return value to -errno, the stub function
should be updated to return a sensible -errno value too.

Stefan

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

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

[Peter Xu]

On Thu, May 24, 2018 at 01:16:11PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, May 24, 2018 at 10:41:09AM +0200, Markus Armbruster wrote:
> >> Regarding the subject: what are "fleids"?
> >
> > Ouch. :( I meant the word "fields".
> 
> Can touch that up in my tree.

[...]

> >> > +    /*
> >> > +     * 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
> >> 
> >> Whoops, misunderstanding!  I meant this line as a placeholder, to
> >> further illustrate my intent.  It should not be committed.  If we need a
> >> comment here, it should use /* traditional comment syntax */.
> >
> > My fault.
> >
> > Please let me know if you want me to repost...  Thanks,
> 
> Can touch that up, too.  No respin needed unless something more complex
> comes up.

I'll modify these two parts in my next post.

Regards,

-- 
Peter Xu

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

[Peter Xu]

On Fri, May 25, 2018 at 10:01:57AM +0100, Stefan Hajnoczi wrote:
> On Fri, May 25, 2018 at 11:30:22AM +0800, Peter Xu wrote:
> > On Thu, May 24, 2018 at 10:28:37AM +0100, Stefan Hajnoczi wrote:
> > > On Thu, May 24, 2018 at 12:39:52PM +0800, Peter Xu wrote:
> > > >  int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> > > >  {
> > > > -#ifndef _WIN32
> > > > +#ifdef _WIN32
> > > > +    return -ENOENT;
> > > 
> > > stubs/fdset.c:monitor_fdset_get_fd() should return -ENOENT instead of -1
> > > now.
> > 
> > Yes that's intended.  That's actually a suggestion from Markus since
> > changing the return code will simplify the code.
> 
> No, I understand that part.  I'm pointing out that
> stubs/fdset.c:monitor_fdset_get_fd() still returns -1 because it was not
> updated by this patch.
> 
> Since this patch changes the return value to -errno, the stub function
> should be updated to return a sensible -errno value too.

Sorry I overlooked.  You are right, I'll touch that up.

Regards,

-- 
Peter Xu

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

[Peter Xu]

On Thu, May 24, 2018 at 11:03:55AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Similar to previous patch, but introduce a new global big lock for
> > mon_fdsets.  Take it where needed.
> 
> The previous patch is "monitor: more comments on lock-free
> fleids/funcs".  Sure you mean that one?

No... I'll remove that sentence directly:

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

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

Okay.

> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c    | 70 +++++++++++++++++++++++++++++++++++++++++-----------
> >  util/osdep.c |  3 ++-
> >  2 files changed, 58 insertions(+), 15 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index f01589f945..6266ff65c4 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -271,6 +271,9 @@ typedef struct QMPRequest QMPRequest;
> >  /* Protects mon_list, monitor_event_state.  */
> 
> Not this patch's problem: there is no monitor_event_state.  Screwed up
> in commit d622cb5879c.  I guess it's monitor_qapi_event_state.

I'll append another patch to do that, and move these fields together.

> 
> >  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;
> 
> Suggest to move mon_fdsets next to the lock protecting it.

Will do.

> 
> > @@ -287,6 +290,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.
> 
> Do you mean CLI parsing?

Yes, will s/param/CLI/.

> 
> > + * Meanwhile it can also be used even at the end of main.  Let's keep
> > + * it initialized for the whole lifecycle of QEMU.
> > + */
> 
> Awkward question, since our main() is such a tangled mess, but here goes
> anyway...  The existing place to initialize monitor.c's globals is
> monitor_init_globals().  But that one runs too late, I guess:
> parse_add_fd() runs earlier, and calls monitor_fdset_add_fd().  Unclean
> even without this lock; no module should be used before its
> initialization function runs.  Sure we can't run monitor_init_globals()
> sufficiently early?

Please see the comment for monitor_init_globals():

    /*
     * Note: qtest_enabled() (which is used in monitor_qapi_event_init())
     * depends on configure_accelerator() above.
     */
    monitor_init_globals();

So I guess it won't work to directly move it earlier.  The init
dependency of QEMU is really complicated.  I'll be fine now that we
mark totally independent init functions (like this one, which is a
mutex init only) as constructor, then we can save some time on
ordering issue.

> 
> > +static void __attribute__((constructor)) mon_fdsets_lock_init(void)
> > +{
> > +    qemu_mutex_init(&mon_fdsets_lock);
> > +}
> > +
> >  /**
> >   * Is @mon a QMP monitor?
> >   */
> > @@ -2316,9 +2329,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,
> > @@ -2353,6 +2368,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;
> > @@ -2372,10 +2388,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);
> > @@ -2391,6 +2409,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;
> > @@ -2420,6 +2439,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;
> >  }
> > @@ -2432,6 +2452,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 */
> > @@ -2452,6 +2473,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 */
> > @@ -2502,16 +2524,21 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> >      fdinfo->fdset_id = mon_fdset->id;
> >      fdinfo->fd = mon_fdset_fd->fd;
> >  
> > +    qemu_mutex_unlock(&mon_fdsets_lock);
> >      return fdinfo;
> >  }
> >  
> >  int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> >  {
> > -#ifndef _WIN32
> > +#ifdef _WIN32
> > +    return -ENOENT;
> 
> ENOENT feels odd, but I guess makes some sense since there is no way to
> add entries.
> 
> > +#else
> >      MonFdset *mon_fdset;
> >      MonFdsetFd *mon_fdset_fd;
> >      int mon_fd_flags;
> > +    int ret = -ENOENT;
> >  
> > +    qemu_mutex_lock(&mon_fdsets_lock);
> >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> >          if (mon_fdset->id != fdset_id) {
> >              continue;
> > @@ -2519,49 +2546,60 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> >          QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
> >              mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
> >              if (mon_fd_flags == -1) {
> > -                return -1;
> > +                ret = -errno;
> > +                goto out;
> >              }
> >  
> >              if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
> > -                return mon_fdset_fd->fd;
> > +                ret = mon_fdset_fd->fd;
> > +                goto out;
> >              }
> >          }
> > -        errno = EACCES;
> > -        return -1;
> > +        ret = -EACCES;
> > +        break;
> >      }
> 
> I still think
> 
>            ret = -EACCES;
>            goto out;
>        }
>        ret = -ENOENT;
> 
>    out:
> 
> would be clearer.

I'll take your advice.

> 
> > +out:
> > +    qemu_mutex_unlock(&mon_fdsets_lock);
> > +    return ret;
> >  #endif
> > -
> > -    errno = ENOENT;
> > -    return -1;
> >  }
> >  
> >  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;
> 
> Dead assignment.  Alternatively,
> 
> > +                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;
> >      }
> 
> ... add
> 
>        ret = -1;
> 
> here, and drop the initializer.  Your choice.

The variable "ret" brought some trouble, so I decided to remove it
directly. :)

@@ -2538,20 +2569,25 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd_dup;

+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
         if (mon_fdset->id != fdset_id) {
             continue;
         }
         QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
             if (mon_fdset_fd_dup->fd == dup_fd) {
-                return -1;
+                goto err;
             }
         }
         mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
         mon_fdset_fd_dup->fd = dup_fd;
         QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
+        qemu_mutex_unlock(&mon_fdsets_lock);
         return 0;
     }
+
+err:
+    qemu_mutex_unlock(&mon_fdsets_lock);
     return -1;
 }

> 
> > -    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) {
> > @@ -2570,14 +2608,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;
> >  }
> 
> Likewise.

I'll do similar thing to drop "ret":

@@ -2560,6 +2596,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd_dup;

+    qemu_mutex_lock(&mon_fdsets_lock);
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
         QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
             if (mon_fdset_fd_dup->fd == dup_fd) {
@@ -2568,13 +2605,17 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
                     if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
                         monitor_fdset_cleanup(mon_fdset);
                     }
-                    return -1;
+                    goto err;
                 } else {
+                    qemu_mutex_unlock(&mon_fdsets_lock);
                     return mon_fdset->id;
                 }
             }
         }
     }
+
+err:
+    qemu_mutex_unlock(&mon_fdsets_lock);
     return -1;
 }

Thanks,

-- 
Peter Xu

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 24, 2018 at 11:03:55AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Similar to previous patch, but introduce a new global big lock for
>> > mon_fdsets.  Take it where needed.
>> 
>> The previous patch is "monitor: more comments on lock-free
>> fleids/funcs".  Sure you mean that one?
>
> No... I'll remove that sentence directly:
>
>   "Introduce a new global big lock for mon_fdsets.  Take it where needed."

Works for me.

>> > The monitor_fdset_get_fd() handling is a bit tricky: now we need to call
>> > qemu_mutex_unlock() which might pollute errno, so we need to make sure
>> > the correct errno be passed up to the callers.  To make things simpler,
>> > we let monitor_fdset_get_fd() return the -errno directly when error
>> > happens, then in qemu_open() we translate it back into errno.
>> 
>> Suggest s/translate/move/.
>
> Okay.
>
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  monitor.c    | 70 +++++++++++++++++++++++++++++++++++++++++-----------
>> >  util/osdep.c |  3 ++-
>> >  2 files changed, 58 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index f01589f945..6266ff65c4 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -271,6 +271,9 @@ typedef struct QMPRequest QMPRequest;
>> >  /* Protects mon_list, monitor_event_state.  */
>> 
>> Not this patch's problem: there is no monitor_event_state.  Screwed up
>> in commit d622cb5879c.  I guess it's monitor_qapi_event_state.
>
> I'll append another patch to do that, and move these fields together.
>
>> 
>> >  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;
>> 
>> Suggest to move mon_fdsets next to the lock protecting it.
>
> Will do.
>
>> 
>> > @@ -287,6 +290,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.
>> 
>> Do you mean CLI parsing?
>
> Yes, will s/param/CLI/.
>
>> 
>> > + * Meanwhile it can also be used even at the end of main.  Let's keep
>> > + * it initialized for the whole lifecycle of QEMU.
>> > + */
>> 
>> Awkward question, since our main() is such a tangled mess, but here goes
>> anyway...  The existing place to initialize monitor.c's globals is
>> monitor_init_globals().  But that one runs too late, I guess:
>> parse_add_fd() runs earlier, and calls monitor_fdset_add_fd().  Unclean
>> even without this lock; no module should be used before its
>> initialization function runs.  Sure we can't run monitor_init_globals()
>> sufficiently early?
>
> Please see the comment for monitor_init_globals():
>
>     /*
>      * Note: qtest_enabled() (which is used in monitor_qapi_event_init())
>      * depends on configure_accelerator() above.
>      */
>     monitor_init_globals();
>
> So I guess it won't work to directly move it earlier.  The init
> dependency of QEMU is really complicated.  I'll be fine now that we
> mark totally independent init functions (like this one, which is a
> mutex init only) as constructor, then we can save some time on
> ordering issue.

Let me rephrase.  There's a preexisting issue: main() calls monitor.c
functions before calling its initialization function
monitor_init_globals().  This needs to be cleaned up.  Would you be
willing to do it?

Possible solutions:

* Perhaps we can move monitor_init_globals() up and/or the code calling
  into monitor.c early down sufficiently.

* Calculate event_clock_type on each use instead of ahead of time.  It's
  qtest_enabled ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME, and neither
  of its users needs to be fast.  Then move monitor_init_globals before
  the code calling into monitor.c.

I'm not opposed to use of constructors for self-contained initialization
(no calls to other modules).  But I don't like initialization spread
over multiple functions.

>> > +static void __attribute__((constructor)) mon_fdsets_lock_init(void)
>> > +{
>> > +    qemu_mutex_init(&mon_fdsets_lock);
>> > +}
>> > +
>> >  /**
>> >   * Is @mon a QMP monitor?
>> >   */
>> > @@ -2316,9 +2329,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,
>> > @@ -2353,6 +2368,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;
>> > @@ -2372,10 +2388,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);
>> > @@ -2391,6 +2409,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;
>> > @@ -2420,6 +2439,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;
>> >  }
>> > @@ -2432,6 +2452,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 */
>> > @@ -2452,6 +2473,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 */
>> > @@ -2502,16 +2524,21 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>> >      fdinfo->fdset_id = mon_fdset->id;
>> >      fdinfo->fd = mon_fdset_fd->fd;
>> >  
>> > +    qemu_mutex_unlock(&mon_fdsets_lock);
>> >      return fdinfo;
>> >  }
>> >  
>> >  int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>> >  {
>> > -#ifndef _WIN32
>> > +#ifdef _WIN32
>> > +    return -ENOENT;
>> 
>> ENOENT feels odd, but I guess makes some sense since there is no way to
>> add entries.
>> 
>> > +#else
>> >      MonFdset *mon_fdset;
>> >      MonFdsetFd *mon_fdset_fd;
>> >      int mon_fd_flags;
>> > +    int ret = -ENOENT;
>> >  
>> > +    qemu_mutex_lock(&mon_fdsets_lock);
>> >      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> >          if (mon_fdset->id != fdset_id) {
>> >              continue;
>> > @@ -2519,49 +2546,60 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>> >          QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
>> >              mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
>> >              if (mon_fd_flags == -1) {
>> > -                return -1;
>> > +                ret = -errno;
>> > +                goto out;
>> >              }
>> >  
>> >              if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
>> > -                return mon_fdset_fd->fd;
>> > +                ret = mon_fdset_fd->fd;
>> > +                goto out;
>> >              }
>> >          }
>> > -        errno = EACCES;
>> > -        return -1;
>> > +        ret = -EACCES;
>> > +        break;
>> >      }
>> 
>> I still think
>> 
>>            ret = -EACCES;
>>            goto out;
>>        }
>>        ret = -ENOENT;
>> 
>>    out:
>> 
>> would be clearer.
>
> I'll take your advice.
>
>> 
>> > +out:
>> > +    qemu_mutex_unlock(&mon_fdsets_lock);
>> > +    return ret;
>> >  #endif
>> > -
>> > -    errno = ENOENT;
>> > -    return -1;
>> >  }
>> >  
>> >  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;
>> 
>> Dead assignment.  Alternatively,
>> 
>> > +                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;
>> >      }
>> 
>> ... add
>> 
>>        ret = -1;
>> 
>> here, and drop the initializer.  Your choice.
>
> The variable "ret" brought some trouble, so I decided to remove it
> directly. :)
>
> @@ -2538,20 +2569,25 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd_dup;
>
> +    qemu_mutex_lock(&mon_fdsets_lock);
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>          if (mon_fdset->id != fdset_id) {
>              continue;
>          }
>          QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
>              if (mon_fdset_fd_dup->fd == dup_fd) {
> -                return -1;
> +                goto err;
>              }
>          }
>          mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
>          mon_fdset_fd_dup->fd = dup_fd;
>          QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
> +        qemu_mutex_unlock(&mon_fdsets_lock);
>          return 0;
>      }
> +
> +err:
> +    qemu_mutex_unlock(&mon_fdsets_lock);
>      return -1;
>  }

Works for me.

>> 
>> > -    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) {
>> > @@ -2570,14 +2608,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;
>> >  }
>> 
>> Likewise.
>
> I'll do similar thing to drop "ret":
>
> @@ -2560,6 +2596,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd_dup;
>
> +    qemu_mutex_lock(&mon_fdsets_lock);
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>          QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
>              if (mon_fdset_fd_dup->fd == dup_fd) {
> @@ -2568,13 +2605,17 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>                      if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
>                          monitor_fdset_cleanup(mon_fdset);
>                      }
> -                    return -1;
> +                    goto err;
>                  } else {
> +                    qemu_mutex_unlock(&mon_fdsets_lock);
>                      return mon_fdset->id;
>                  }
>              }
>          }
>      }
> +
> +err:
> +    qemu_mutex_unlock(&mon_fdsets_lock);
>      return -1;
>  }

Also works for me.

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

[Peter Xu]

On Mon, May 28, 2018 at 05:19:08PM +0200, Markus Armbruster wrote:

[...]

> >> 
> >> > + * Meanwhile it can also be used even at the end of main.  Let's keep
> >> > + * it initialized for the whole lifecycle of QEMU.
> >> > + */
> >> 
> >> Awkward question, since our main() is such a tangled mess, but here goes
> >> anyway...  The existing place to initialize monitor.c's globals is
> >> monitor_init_globals().  But that one runs too late, I guess:
> >> parse_add_fd() runs earlier, and calls monitor_fdset_add_fd().  Unclean
> >> even without this lock; no module should be used before its
> >> initialization function runs.  Sure we can't run monitor_init_globals()
> >> sufficiently early?
> >
> > Please see the comment for monitor_init_globals():
> >
> >     /*
> >      * Note: qtest_enabled() (which is used in monitor_qapi_event_init())
> >      * depends on configure_accelerator() above.
> >      */
> >     monitor_init_globals();
> >
> > So I guess it won't work to directly move it earlier.  The init
> > dependency of QEMU is really complicated.  I'll be fine now that we
> > mark totally independent init functions (like this one, which is a
> > mutex init only) as constructor, then we can save some time on
> > ordering issue.
> 
> Let me rephrase.  There's a preexisting issue: main() calls monitor.c
> functions before calling its initialization function
> monitor_init_globals().  This needs to be cleaned up.  Would you be
> willing to do it?
> 
> Possible solutions:
> 
> * Perhaps we can move monitor_init_globals() up and/or the code calling
>   into monitor.c early down sufficiently.
> 
> * Calculate event_clock_type on each use instead of ahead of time.  It's
>   qtest_enabled ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME, and neither
>   of its users needs to be fast.  Then move monitor_init_globals before
>   the code calling into monitor.c.

Indeed.  Obviously you thought a step further. :)

> 
> I'm not opposed to use of constructors for self-contained initialization
> (no calls to other modules).  But I don't like initialization spread
> over multiple functions.

Since this work will actually decide where I should init this new
fdset lock, so I'll try to do that altogether within the series.

Thanks for your suggestions!  It makes sense.

-- 
Peter Xu