Monitor code now can be run in more than one thread.  Let it be thread
safe when accessing suspend_cnt counter.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index 35d8925636..6ac1b2065d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3874,7 +3874,7 @@ static int monitor_can_read(void *opaque)
 {
     Monitor *mon = opaque;
 
-    return (mon->suspend_cnt == 0) ? 1 : 0;
+    return (atomic_read(&mon->suspend_cnt) == 0) ? 1 : 0;
 }
 
 /*
@@ -4003,7 +4003,7 @@ int monitor_suspend(Monitor *mon)
 {
     if (!mon->rs)
         return -ENOTTY;
-    mon->suspend_cnt++;
+    atomic_inc(&mon->suspend_cnt);
     return 0;
 }
 
@@ -4011,8 +4011,9 @@ void monitor_resume(Monitor *mon)
 {
     if (!mon->rs)
         return;
-    if (--mon->suspend_cnt == 0)
+    if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
         readline_show_prompt(mon->rs);
+    }
 }
 
 static QObject *get_qmp_greeting(void)
@@ -4078,19 +4079,19 @@ static void monitor_event(void *opaque, int event)
             monitor_resume(mon);
             monitor_flush(mon);
         } else {
-            mon->suspend_cnt = 0;
+            atomic_set(&mon->suspend_cnt, 0);
         }
         break;
 
     case CHR_EVENT_MUX_OUT:
         if (mon->reset_seen) {
-            if (mon->suspend_cnt == 0) {
+            if (atomic_read(&mon->suspend_cnt) == 0) {
                 monitor_printf(mon, "\n");
             }
             monitor_flush(mon);
             monitor_suspend(mon);
         } else {
-            mon->suspend_cnt++;
+            atomic_inc(&mon->suspend_cnt);
         }
         qemu_mutex_lock(&mon->out_lock);
         mon->mux_out = 1;
-- 
2.14.3

On Tue, Dec 05, 2017 at 01:51:49PM +0800, Peter Xu wrote:
> Monitor code now can be run in more than one thread.  Let it be thread
> safe when accessing suspend_cnt counter.

Please add doc comments to the functions explaining which thread they
may be called from (especially public functions).  Without this
information other people will have a hard time keeping code thread-safe.

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

From docs/devel/atomics.txt:

  Macros defined by qemu/atomic.h fall in three camps:

  - compiler barriers: barrier();

  - weak atomic access and manual memory barriers: atomic_read(),
    atomic_set(), smp_rmb(), smp_wmb(), smp_mb(), smp_mb_acquire(),
    smp_mb_release(), smp_read_barrier_depends();

  - sequentially consistent atomic access: everything else.

The atomic_read() and atomic_set() calls in this patch aren't effective
without memory barriers.  Please use atomic_mb_read() and
atomic_mb_set() instead.

On Wed, Dec 13, 2017 at 06:43:58PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 05, 2017 at 01:51:49PM +0800, Peter Xu wrote:
> > Monitor code now can be run in more than one thread.  Let it be thread
> > safe when accessing suspend_cnt counter.
> 
> Please add doc comments to the functions explaining which thread they
> may be called from (especially public functions).  Without this
> information other people will have a hard time keeping code thread-safe.

The two public functions would be monitor_resume() and
monitor_suspend().  IIUC as long as these functions are using atomic
operations correctly they should be able to be called anywhere?

Could you help point out what I missed?

> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> From docs/devel/atomics.txt:
> 
>   Macros defined by qemu/atomic.h fall in three camps:
> 
>   - compiler barriers: barrier();
> 
>   - weak atomic access and manual memory barriers: atomic_read(),
>     atomic_set(), smp_rmb(), smp_wmb(), smp_mb(), smp_mb_acquire(),
>     smp_mb_release(), smp_read_barrier_depends();
> 
>   - sequentially consistent atomic access: everything else.
> 
> The atomic_read() and atomic_set() calls in this patch aren't effective
> without memory barriers.  Please use atomic_mb_read() and
> atomic_mb_set() instead.

Will do.  Thanks,

-- 
Peter Xu

On Sat, Dec 16, 2017 at 02:12:11PM +0800, Peter Xu wrote:
> On Wed, Dec 13, 2017 at 06:43:58PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Dec 05, 2017 at 01:51:49PM +0800, Peter Xu wrote:
> > > Monitor code now can be run in more than one thread.  Let it be thread
> > > safe when accessing suspend_cnt counter.
> > 
> > Please add doc comments to the functions explaining which thread they
> > may be called from (especially public functions).  Without this
> > information other people will have a hard time keeping code thread-safe.
> 
> The two public functions would be monitor_resume() and
> monitor_suspend().  IIUC as long as these functions are using atomic
> operations correctly they should be able to be called anywhere?
> 
> Could you help point out what I missed?

I don't mean just the public functions.  How are other people supposed
to know which monitor function executes in which thread without first
studying all the monitor code?  This is where comments, naming schemes,
or grouping functions helps.

Not every function needs to be documented in detail, but there needs to
be some structure to help people understand what happens in the IOThread
and what happens in the main loop.

Just adding threading to the existing code without leaving clues for
people who come after you is likely to result in future bugs.

Stefan

On Sat, Dec 16, 2017 at 09:11:08AM +0000, Stefan Hajnoczi wrote:
> On Sat, Dec 16, 2017 at 02:12:11PM +0800, Peter Xu wrote:
> > On Wed, Dec 13, 2017 at 06:43:58PM +0000, Stefan Hajnoczi wrote:
> > > On Tue, Dec 05, 2017 at 01:51:49PM +0800, Peter Xu wrote:
> > > > Monitor code now can be run in more than one thread.  Let it be thread
> > > > safe when accessing suspend_cnt counter.
> > > 
> > > Please add doc comments to the functions explaining which thread they
> > > may be called from (especially public functions).  Without this
> > > information other people will have a hard time keeping code thread-safe.
> > 
> > The two public functions would be monitor_resume() and
> > monitor_suspend().  IIUC as long as these functions are using atomic
> > operations correctly they should be able to be called anywhere?
> > 
> > Could you help point out what I missed?
> 
> I don't mean just the public functions.  How are other people supposed
> to know which monitor function executes in which thread without first
> studying all the monitor code?  This is where comments, naming schemes,
> or grouping functions helps.
> 
> Not every function needs to be documented in detail, but there needs to
> be some structure to help people understand what happens in the IOThread
> and what happens in the main loop.
> 
> Just adding threading to the existing code without leaving clues for
> people who come after you is likely to result in future bugs.

Yes, I fully agree with you that we need to do better commenting when
doing things like this.

I am just not sure what you would prefer me to add since AFAICT these
functions (after with the atomic protections) are really free to be
called anywhere, and also in any thread (even newly created threads
rather than main thread or monitor IO thread), and they are free of
locks too, like monitor_lock.  Here I only mean monitor_suspend() and
monitor_resume() for sure.  For the rest of functions, IMHO since they
are internal to monitor code so people who will call them do need to
know something about monitors.

I would be more appreciated if you can provide some examples here to
show what you mean.  Thanks,

-- 
Peter Xu