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

[Peter Xu <peterx@redhat.com>]

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,

-- 
Peter Xu