diff mbox series

[RFC,v4,15/27] monitor: let monitor_{suspend|resume} thread safe

Message ID 20171116130610.23582-16-peterx@redhat.com
State New
Headers show
Series QMP: out-of-band (OOB) execution support | expand

Commit Message

Peter Xu Nov. 16, 2017, 1:05 p.m. UTC 
Monitor code now can be run in more than one thread.  Let the suspend
and resume code be thread safe.

Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Dr. David Alan Gilbert Nov. 23, 2017, 11:23 a.m. UTC | #1 
* Peter Xu (peterx@redhat.com) wrote:
> Monitor code now can be run in more than one thread.  Let the suspend
> and resume code be thread safe.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index ec03f1b232..30f9cd80de 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -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)

Do you also need to do an atomic_read() in monitor_can_read?
(I guess the code in monitor_event is ok because it's only the mux
path, but still it's probably better to use all atomic ops on
the suspend_cnt variable to be consistent).

Dave

> -- 
> 2.13.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Peter Xu Nov. 24, 2017, 7:58 a.m. UTC | #2 
On Thu, Nov 23, 2017 at 11:23:16AM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > Monitor code now can be run in more than one thread.  Let the suspend
> > and resume code be thread safe.
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index ec03f1b232..30f9cd80de 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -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)
> 
> Do you also need to do an atomic_read() in monitor_can_read?
> (I guess the code in monitor_event is ok because it's only the mux
> path, but still it's probably better to use all atomic ops on
> the suspend_cnt variable to be consistent).

Agreed.
diff mbox series

Patch

diff --git a/monitor.c b/monitor.c
index ec03f1b232..30f9cd80de 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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)