| Message ID | 20180509041734.14135-2-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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]
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,
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: