* [Qemu-devel] [PATCH v2 1/3] monitor: rename out_lock to mon_lock
2018-04-18 9:02 [Qemu-devel] [PATCH v2 0/3] monitor: let Monitor be thread safe Peter Xu
@ 2018-04-18 9:02 ` Peter Xu
2018-04-30 9:56 ` Stefan Hajnoczi
2018-04-18 9:02 ` [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper Peter Xu
2018-04-18 9:02 ` [Qemu-devel] [PATCH v2 3/3] monitor: add lock to protect mon_fdsets Peter Xu
2 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2018-04-18 9:02 UTC (permalink / raw)
To: qemu-devel
Cc: peterx, Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert
The out_lock was only protecting out buffers. 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. We split
this lock until necessary. So far I don't see a reason to complicate
lock usage for monitors.
Since at it, arrange the Monitor struct a bit.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 39 ++++++++++++++++++---------------------
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/monitor.c b/monitor.c
index 39f8ee17ba..c93aa4e22b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -202,20 +202,17 @@ typedef struct {
struct Monitor {
CharBackend chr;
+ /* We can't access guest memory when holding the lock */
+ QemuMutex mon_lock;
int reset_seen;
int flags;
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. */
+ /* Read under either BQL or mon_lock, written with BQL+mon_lock. */
int mux_out;
-
ReadLineState *rs;
MonitorQMP qmp;
gchar *mon_cpu_path;
@@ -366,14 +363,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 +408,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 +418,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 +431,7 @@ static void monitor_puts(Monitor *mon, const char *str)
monitor_flush_locked(mon);
}
}
- qemu_mutex_unlock(&mon->out_lock);
+ qemu_mutex_unlock(&mon->mon_lock);
}
void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
@@ -728,7 +725,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
bool use_io_thr)
{
memset(mon, 0, sizeof(Monitor));
- qemu_mutex_init(&mon->out_lock);
+ qemu_mutex_init(&mon->mon_lock);
qemu_mutex_init(&mon->qmp.qmp_queue_lock);
mon->outbuf = qstring_new();
/* Use *mon_cmds by default. */
@@ -748,7 +745,7 @@ static void monitor_data_destroy(Monitor *mon)
}
readline_free(mon->rs);
QDECREF(mon->outbuf);
- qemu_mutex_destroy(&mon->out_lock);
+ qemu_mutex_destroy(&mon->mon_lock);
qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
monitor_qmp_cleanup_req_queue_locked(mon);
monitor_qmp_cleanup_resp_queue_locked(mon);
@@ -780,13 +777,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
handle_hmp_command(&hmp, command_line);
cur_mon = old_mon;
- qemu_mutex_lock(&hmp.out_lock);
+ qemu_mutex_lock(&hmp.mon_lock);
if (qstring_get_length(hmp.outbuf) > 0) {
output = g_strdup(qstring_get_str(hmp.outbuf));
} else {
output = g_strdup("");
}
- qemu_mutex_unlock(&hmp.out_lock);
+ qemu_mutex_unlock(&hmp.mon_lock);
out:
monitor_data_destroy(&hmp);
@@ -4383,9 +4380,9 @@ static void monitor_event(void *opaque, int event)
switch (event) {
case CHR_EVENT_MUX_IN:
- qemu_mutex_lock(&mon->out_lock);
+ qemu_mutex_lock(&mon->mon_lock);
mon->mux_out = 0;
- qemu_mutex_unlock(&mon->out_lock);
+ qemu_mutex_unlock(&mon->mon_lock);
if (mon->reset_seen) {
readline_restart(mon->rs);
monitor_resume(mon);
@@ -4405,9 +4402,9 @@ static void monitor_event(void *opaque, int event)
} else {
atomic_inc(&mon->suspend_cnt);
}
- qemu_mutex_lock(&mon->out_lock);
+ qemu_mutex_lock(&mon->mon_lock);
mon->mux_out = 1;
- qemu_mutex_unlock(&mon->out_lock);
+ qemu_mutex_unlock(&mon->mon_lock);
break;
case CHR_EVENT_OPENED:
--
2.14.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] monitor: rename out_lock to mon_lock
2018-04-18 9:02 ` [Qemu-devel] [PATCH v2 1/3] monitor: rename out_lock to mon_lock Peter Xu
@ 2018-04-30 9:56 ` Stefan Hajnoczi
2018-05-02 6:33 ` Peter Xu
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2018-04-30 9:56 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Marc-André Lureau,
Markus Armbruster, Dr . David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]
On Wed, Apr 18, 2018 at 05:02:37PM +0800, Peter Xu wrote:
> The out_lock was only protecting out buffers. 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. We split
> this lock until necessary. So far I don't see a reason to complicate
> lock usage for monitors.
>
> Since at it, arrange the Monitor struct a bit.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> monitor.c | 39 ++++++++++++++++++---------------------
> 1 file changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 39f8ee17ba..c93aa4e22b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -202,20 +202,17 @@ typedef struct {
>
> struct Monitor {
> CharBackend chr;
> + /* We can't access guest memory when holding the lock */
> + QemuMutex mon_lock;
Please document which field this lock protects. For example, outbuf and
out_watch need a comment.
> int reset_seen;
> int flags;
> 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;
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] monitor: rename out_lock to mon_lock
2018-04-30 9:56 ` Stefan Hajnoczi
@ 2018-05-02 6:33 ` Peter Xu
0 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2018-05-02 6:33 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Eric Blake, Marc-André Lureau,
Markus Armbruster, Dr . David Alan Gilbert
On Mon, Apr 30, 2018 at 10:56:49AM +0100, Stefan Hajnoczi wrote:
> On Wed, Apr 18, 2018 at 05:02:37PM +0800, Peter Xu wrote:
> > The out_lock was only protecting out buffers. 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. We split
> > this lock until necessary. So far I don't see a reason to complicate
> > lock usage for monitors.
> >
> > Since at it, arrange the Monitor struct a bit.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > monitor.c | 39 ++++++++++++++++++---------------------
> > 1 file changed, 18 insertions(+), 21 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 39f8ee17ba..c93aa4e22b 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -202,20 +202,17 @@ typedef struct {
> >
> > struct Monitor {
> > CharBackend chr;
> > + /* We can't access guest memory when holding the lock */
> > + QemuMutex mon_lock;
>
> Please document which field this lock protects. For example, outbuf and
> out_watch need a comment.
Sure!
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper
2018-04-18 9:02 [Qemu-devel] [PATCH v2 0/3] monitor: let Monitor be thread safe Peter Xu
2018-04-18 9:02 ` [Qemu-devel] [PATCH v2 1/3] monitor: rename out_lock to mon_lock Peter Xu
@ 2018-04-18 9:02 ` Peter Xu
2018-04-30 10:10 ` Stefan Hajnoczi
2018-04-18 9:02 ` [Qemu-devel] [PATCH v2 3/3] monitor: add lock to protect mon_fdsets Peter Xu
2 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2018-04-18 9:02 UTC (permalink / raw)
To: qemu-devel
Cc: peterx, Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert
Went through all the montior.h APIs to make sure existing Monitor
related APIs will always take the new monitor lock when necessary.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/monitor.c b/monitor.c
index c93aa4e22b..f4951cafbc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
if (!mon->rs)
return;
+ qemu_mutex_lock(&mon->mon_lock);
readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
if (show_prompt)
readline_show_prompt(mon->rs);
+ qemu_mutex_unlock(&mon->mon_lock);
}
int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
void *opaque)
{
if (mon->rs) {
+ qemu_mutex_lock(&mon->mon_lock);
readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
+ qemu_mutex_unlock(&mon->mon_lock);
/* prompt is printed on return from the command handler */
return 0;
} else {
@@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
cur_mon->qmp.commands = &qmp_commands;
}
-/* set the current CPU defined by the user */
-int monitor_set_cpu(int cpu_index)
+static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)
{
CPUState *cpu;
@@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
if (cpu == NULL) {
return -1;
}
- g_free(cur_mon->mon_cpu_path);
- cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
+ g_free(mon->mon_cpu_path);
+ mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
return 0;
}
+/* set the current CPU defined by the user */
+int monitor_set_cpu(int cpu_index)
+{
+ int ret;
+
+ qemu_mutex_lock(&cur_mon->mon_lock);
+ ret = monitor_set_cpu_locked(cur_mon, cpu_index);
+ qemu_mutex_unlock(&cur_mon->mon_lock);
+
+ return ret;
+}
+
static CPUState *mon_get_cpu_sync(bool synchronize)
{
CPUState *cpu;
+ qemu_mutex_lock(&cur_mon->mon_lock);
if (cur_mon->mon_cpu_path) {
cpu = (CPUState *) object_resolve_path_type(cur_mon->mon_cpu_path,
TYPE_CPU, NULL);
@@ -1336,11 +1352,14 @@ static CPUState *mon_get_cpu_sync(bool synchronize)
}
if (!cur_mon->mon_cpu_path) {
if (!first_cpu) {
+ qemu_mutex_unlock(&cur_mon->mon_lock);
return NULL;
}
- monitor_set_cpu(first_cpu->cpu_index);
+ monitor_set_cpu_locked(cur_mon, first_cpu->cpu_index);
cpu = first_cpu;
}
+ qemu_mutex_unlock(&cur_mon->mon_lock);
+
if (synchronize) {
cpu_synchronize_state(cpu);
}
@@ -2239,6 +2258,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;
@@ -2252,9 +2272,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
QLIST_REMOVE(monfd, next);
g_free(monfd->name);
g_free(monfd);
-
+ qemu_mutex_unlock(&mon->mon_lock);
return fd;
}
+ qemu_mutex_unlock(&mon->mon_lock);
error_setg(errp, "File descriptor named '%s' has not been found", fdname);
return -1;
--
2.14.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper
2018-04-18 9:02 ` [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper Peter Xu
@ 2018-04-30 10:10 ` Stefan Hajnoczi
2018-05-02 7:04 ` Peter Xu
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2018-04-30 10:10 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Marc-André Lureau,
Markus Armbruster, Dr . David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 3788 bytes --]
On Wed, Apr 18, 2018 at 05:02:38PM +0800, Peter Xu wrote:
> diff --git a/monitor.c b/monitor.c
> index c93aa4e22b..f4951cafbc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
> if (!mon->rs)
> return;
>
> + qemu_mutex_lock(&mon->mon_lock);
> readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
> if (show_prompt)
> readline_show_prompt(mon->rs);
> + qemu_mutex_unlock(&mon->mon_lock);
> }
>
> int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
> void *opaque)
> {
> if (mon->rs) {
> + qemu_mutex_lock(&mon->mon_lock);
> readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
> + qemu_mutex_unlock(&mon->mon_lock);
> /* prompt is printed on return from the command handler */
> return 0;
> } else {
I'm not sure why the lock is being used around readline_start() and
readline_show_prompt(). There are other readline_*() callers who do not
take the lock, which is suspicious.
Can you explain the purpose of this?
> @@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
> cur_mon->qmp.commands = &qmp_commands;
> }
>
> -/* set the current CPU defined by the user */
> -int monitor_set_cpu(int cpu_index)
> +static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)
This function requires the BQL since qemu_get_cpu() accesses the cpus
list without acquiring qemu_cpu_list_lock.
Two options:
1. Document that monitor_set_cpu() must be called with the BQL held.
2. Audit qemu_cpu_list_lock to check that it meets the out-of-band
monitor code requirements, document that qemu_cpu_list_lock code must
follow out-of-band monitor code requirements, and then take the lock.
#1 is more practical since we will probably never need to call
monitor_set_cpu() from out-of-band monitor code. Anyway, in that case
mon_lock is not needed unless there is a mon field that needs to be
protected.
> {
> CPUState *cpu;
>
> @@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
> if (cpu == NULL) {
> return -1;
> }
> - g_free(cur_mon->mon_cpu_path);
> - cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> + g_free(mon->mon_cpu_path);
> + mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> return 0;
> }
>
> +/* set the current CPU defined by the user */
> +int monitor_set_cpu(int cpu_index)
> +{
> + int ret;
> +
> + qemu_mutex_lock(&cur_mon->mon_lock);
> + ret = monitor_set_cpu_locked(cur_mon, cpu_index);
> + qemu_mutex_unlock(&cur_mon->mon_lock);
> +
> + return ret;
> +}
> +
> static CPUState *mon_get_cpu_sync(bool synchronize)
> {
This function calls monitor_set_cpu() so it must be called from the BQL.
The locking changes are probably not needed. This function just needs
to be documented as BQL-only.
> @@ -2239,6 +2258,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;
>
> @@ -2252,9 +2272,10 @@ 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);
What about all the other mon->fds users? They need to lock too,
otherwise we will QLIST_REMOVE() an fd while they are accessing the
list!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper
2018-04-30 10:10 ` Stefan Hajnoczi
@ 2018-05-02 7:04 ` Peter Xu
0 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2018-05-02 7:04 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Eric Blake, Marc-André Lureau,
Markus Armbruster, Dr . David Alan Gilbert
On Mon, Apr 30, 2018 at 11:10:50AM +0100, Stefan Hajnoczi wrote:
> On Wed, Apr 18, 2018 at 05:02:38PM +0800, Peter Xu wrote:
> > diff --git a/monitor.c b/monitor.c
> > index c93aa4e22b..f4951cafbc 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
> > if (!mon->rs)
> > return;
> >
> > + qemu_mutex_lock(&mon->mon_lock);
> > readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
> > if (show_prompt)
> > readline_show_prompt(mon->rs);
> > + qemu_mutex_unlock(&mon->mon_lock);
> > }
> >
> > int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
> > void *opaque)
> > {
> > if (mon->rs) {
> > + qemu_mutex_lock(&mon->mon_lock);
> > readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
> > + qemu_mutex_unlock(&mon->mon_lock);
> > /* prompt is printed on return from the command handler */
> > return 0;
> > } else {
>
> I'm not sure why the lock is being used around readline_start() and
> readline_show_prompt(). There are other readline_*() callers who do not
> take the lock, which is suspicious.
>
> Can you explain the purpose of this?
>
> > @@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
> > cur_mon->qmp.commands = &qmp_commands;
> > }
> >
> > -/* set the current CPU defined by the user */
> > -int monitor_set_cpu(int cpu_index)
> > +static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)
>
> This function requires the BQL since qemu_get_cpu() accesses the cpus
> list without acquiring qemu_cpu_list_lock.
>
> Two options:
> 1. Document that monitor_set_cpu() must be called with the BQL held.
> 2. Audit qemu_cpu_list_lock to check that it meets the out-of-band
> monitor code requirements, document that qemu_cpu_list_lock code must
> follow out-of-band monitor code requirements, and then take the lock.
>
> #1 is more practical since we will probably never need to call
> monitor_set_cpu() from out-of-band monitor code. Anyway, in that case
> mon_lock is not needed unless there is a mon field that needs to be
> protected.
You are right.
After a second thought I think readline is not needed to be protected.
IMHO it's only used in parsing phase, so actually we don't have
multi-threading issue with that (parsing is either happening in main
thread only, or monitor iothread only).
So I'll drop all the readline_* protections, and add a comment for
monitor_set_cpu() on BQL.
>
> > {
> > CPUState *cpu;
> >
> > @@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
> > if (cpu == NULL) {
> > return -1;
> > }
> > - g_free(cur_mon->mon_cpu_path);
> > - cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> > + g_free(mon->mon_cpu_path);
> > + mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> > return 0;
> > }
> >
> > +/* set the current CPU defined by the user */
> > +int monitor_set_cpu(int cpu_index)
> > +{
> > + int ret;
> > +
> > + qemu_mutex_lock(&cur_mon->mon_lock);
> > + ret = monitor_set_cpu_locked(cur_mon, cpu_index);
> > + qemu_mutex_unlock(&cur_mon->mon_lock);
> > +
> > + return ret;
> > +}
> > +
> > static CPUState *mon_get_cpu_sync(bool synchronize)
> > {
>
> This function calls monitor_set_cpu() so it must be called from the BQL.
> The locking changes are probably not needed. This function just needs
> to be documented as BQL-only.
Yes. Will do.
>
> > @@ -2239,6 +2258,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;
> >
> > @@ -2252,9 +2272,10 @@ 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);
>
> What about all the other mon->fds users? They need to lock too,
> otherwise we will QLIST_REMOVE() an fd while they are accessing the
> list!
Indeed! I think I'll drop most of this patch, only add protection for
mon->fds, and add those comments that you suggested. They make sense
to me. Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] monitor: add lock to protect mon_fdsets
2018-04-18 9:02 [Qemu-devel] [PATCH v2 0/3] monitor: let Monitor be thread safe Peter Xu
2018-04-18 9:02 ` [Qemu-devel] [PATCH v2 1/3] monitor: rename out_lock to mon_lock Peter Xu
2018-04-18 9:02 ` [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper Peter Xu
@ 2018-04-18 9:02 ` Peter Xu
2018-04-30 10:21 ` Stefan Hajnoczi
2 siblings, 1 reply; 10+ messages in thread
From: Peter Xu @ 2018-04-18 9:02 UTC (permalink / raw)
To: qemu-devel
Cc: peterx, Eric Blake, Marc-André Lureau, Markus Armbruster,
Stefan Hajnoczi, Dr . David Alan Gilbert
Similar to previous patch, but introduce a new global big lock for
mon_fdsets. Take it where needed.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 42 insertions(+), 6 deletions(-)
diff --git a/monitor.c b/monitor.c
index f4951cafbc..40b5b56f66 100644
--- a/monitor.c
+++ b/monitor.c
@@ -254,6 +254,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;
@@ -270,6 +273,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?
*/
@@ -2308,9 +2321,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,
@@ -2345,6 +2360,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;
@@ -2364,10 +2380,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);
@@ -2383,6 +2401,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;
@@ -2412,6 +2431,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;
}
@@ -2424,6 +2444,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 */
@@ -2444,6 +2465,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 */
@@ -2494,6 +2516,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;
}
@@ -2531,29 +2554,38 @@ 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) {
@@ -2562,14 +2594,18 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
monitor_fdset_cleanup(mon_fdset);
}
- return -1;
+ ret = -1;
+ goto out;
} else {
- return mon_fdset->id;
+ ret = mon_fdset->id;
+ goto out;
}
}
}
}
- return -1;
+out:
+ qemu_mutex_unlock(&mon_fdsets_lock);
+ return ret;
}
int monitor_fdset_dup_fd_find(int dup_fd)
--
2.14.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] monitor: add lock to protect mon_fdsets
2018-04-18 9:02 ` [Qemu-devel] [PATCH v2 3/3] monitor: add lock to protect mon_fdsets Peter Xu
@ 2018-04-30 10:21 ` Stefan Hajnoczi
2018-05-02 7:22 ` Peter Xu
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Hajnoczi @ 2018-04-30 10:21 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Marc-André Lureau,
Markus Armbruster, Dr . David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 211 bytes --]
On Wed, Apr 18, 2018 at 05:02:39PM +0800, Peter Xu wrote:
> Similar to previous patch, but introduce a new global big lock for
> mon_fdsets. Take it where needed.
Looks like monitor_fdset_get_fd() is missing.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] monitor: add lock to protect mon_fdsets
2018-04-30 10:21 ` Stefan Hajnoczi
@ 2018-05-02 7:22 ` Peter Xu
0 siblings, 0 replies; 10+ messages in thread
From: Peter Xu @ 2018-05-02 7:22 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Eric Blake, Marc-André Lureau,
Markus Armbruster, Dr . David Alan Gilbert
On Mon, Apr 30, 2018 at 11:21:26AM +0100, Stefan Hajnoczi wrote:
> On Wed, Apr 18, 2018 at 05:02:39PM +0800, Peter Xu wrote:
> > Similar to previous patch, but introduce a new global big lock for
> > mon_fdsets. Take it where needed.
>
> Looks like monitor_fdset_get_fd() is missing.
Yes. Fixing that up. Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 10+ messages in thread