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

[Markus Armbruster <armbru@redhat.com>]

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]