diff mbox series

[v7,1/4] monitor: rename out_lock to mon_lock

Message ID 20180524043952.11245-2-peterx@redhat.com
State New
Headers show
Series monitor: let Monitor be thread safe | expand

Commit Message

Peter Xu May 24, 2018, 4:39 a.m. UTC 
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(-)

Comments

Markus Armbruster May 24, 2018, 8:29 a.m. UTC | #1 
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>
Peter Xu May 24, 2018, 8:45 a.m. UTC | #2 
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!
Markus Armbruster May 24, 2018, 11:14 a.m. UTC | #3 
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!
diff mbox series

Patch

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: