[Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Peter Xu]

In the future the monitor iothread may be accessing the cur_mon as
well (via monitor_qmp_dispatch_one()).  Before we introduce a real
Out-Of-Band command, let's convert the cur_mon variable to be a
per-thread variable to make sure there won't be a race between threads.

Note that thread variables are not initialized to a valid value when new
thread is created.  However for our case we don't need to set it up,
since the cur_mon variable is only used in such a pattern:

  old_mon = cur_mon;
  cur_mon = xxx;
  (do something, read cur_mon if necessary in the stack)
  cur_mon = old_mon;

It plays a role as stack variable, so no need to be initialized at all.
We only need to make sure the variable won't be changed unexpectedly by
other threads.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
v3:
- fix code style warning from patchew
v2:
- drop qemu-thread changes
---
 include/monitor/monitor.h | 2 +-
 monitor.c                 | 2 +-
 stubs/monitor.c           | 2 +-
 tests/test-util-sockets.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index d6ab70cae2..2ef5e04b37 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -6,7 +6,7 @@
 #include "qapi/qapi-types-misc.h"
 #include "qemu/readline.h"
 
-extern Monitor *cur_mon;
+extern __thread Monitor *cur_mon;
 
 /* flags for monitor_init */
 /* 0x01 unused */
diff --git a/monitor.c b/monitor.c
index 39f8ee17ba..c90c31b6b3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -266,7 +266,7 @@ static mon_cmd_t info_cmds[];
 
 QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
 
-Monitor *cur_mon;
+__thread Monitor *cur_mon;
 
 static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME;
 
diff --git a/stubs/monitor.c b/stubs/monitor.c
index e018c8f594..3890771bb5 100644
--- a/stubs/monitor.c
+++ b/stubs/monitor.c
@@ -3,7 +3,7 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 
-Monitor *cur_mon = NULL;
+__thread Monitor *cur_mon;
 
 int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
 {
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index acadd85e8f..6195a3ac36 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -69,7 +69,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
  * stubs/monitor.c is defined, to make sure monitor.o is discarded
  * otherwise we get duplicate syms at link time.
  */
-Monitor *cur_mon;
+__thread Monitor *cur_mon;
 void monitor_init(Chardev *chr, int flags) {}
 
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

On 04/12/2018 01:11 AM, Peter Xu wrote:
> In the future the monitor iothread may be accessing the cur_mon as
> well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> Out-Of-Band command, let's convert the cur_mon variable to be a
> per-thread variable to make sure there won't be a race between threads.
> 
> Note that thread variables are not initialized to a valid value when new
> thread is created.  However for our case we don't need to set it up,
> since the cur_mon variable is only used in such a pattern:
> 
>   old_mon = cur_mon;
>   cur_mon = xxx;
>   (do something, read cur_mon if necessary in the stack)
>   cur_mon = old_mon;
> 
> It plays a role as stack variable, so no need to be initialized at all.
> We only need to make sure the variable won't be changed unexpectedly by
> other threads.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Marc-André Lureau]

On Thu, Apr 12, 2018 at 8:11 AM, Peter Xu <peterx@redhat.com> wrote:
> In the future the monitor iothread may be accessing the cur_mon as
> well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> Out-Of-Band command, let's convert the cur_mon variable to be a
> per-thread variable to make sure there won't be a race between threads.
>
> Note that thread variables are not initialized to a valid value when new
> thread is created.  However for our case we don't need to set it up,
> since the cur_mon variable is only used in such a pattern:
>
>   old_mon = cur_mon;
>   cur_mon = xxx;
>   (do something, read cur_mon if necessary in the stack)
>   cur_mon = old_mon;
>
> It plays a role as stack variable, so no need to be initialized at all.
> We only need to make sure the variable won't be changed unexpectedly by
> other threads.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
> v3:
> - fix code style warning from patchew
> v2:
> - drop qemu-thread changes
> ---
>  include/monitor/monitor.h | 2 +-
>  monitor.c                 | 2 +-
>  stubs/monitor.c           | 2 +-
>  tests/test-util-sockets.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index d6ab70cae2..2ef5e04b37 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -6,7 +6,7 @@
>  #include "qapi/qapi-types-misc.h"
>  #include "qemu/readline.h"
>
> -extern Monitor *cur_mon;
> +extern __thread Monitor *cur_mon;
>
>  /* flags for monitor_init */
>  /* 0x01 unused */
> diff --git a/monitor.c b/monitor.c
> index 39f8ee17ba..c90c31b6b3 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -266,7 +266,7 @@ static mon_cmd_t info_cmds[];
>
>  QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>
> -Monitor *cur_mon;
> +__thread Monitor *cur_mon;
>
>  static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME;
>
> diff --git a/stubs/monitor.c b/stubs/monitor.c
> index e018c8f594..3890771bb5 100644
> --- a/stubs/monitor.c
> +++ b/stubs/monitor.c
> @@ -3,7 +3,7 @@
>  #include "qemu-common.h"
>  #include "monitor/monitor.h"
>
> -Monitor *cur_mon = NULL;
> +__thread Monitor *cur_mon;
>
>  int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
>  {
> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
> index acadd85e8f..6195a3ac36 100644
> --- a/tests/test-util-sockets.c
> +++ b/tests/test-util-sockets.c
> @@ -69,7 +69,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>   * stubs/monitor.c is defined, to make sure monitor.o is discarded
>   * otherwise we get duplicate syms at link time.
>   */
> -Monitor *cur_mon;
> +__thread Monitor *cur_mon;
>  void monitor_init(Chardev *chr, int flags) {}
>
>
> --
> 2.14.3
>

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]

On Thu, Apr 12, 2018 at 02:11:08PM +0800, Peter Xu wrote:
> In the future the monitor iothread may be accessing the cur_mon as
> well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> Out-Of-Band command, let's convert the cur_mon variable to be a
> per-thread variable to make sure there won't be a race between threads.
>
> Note that thread variables are not initialized to a valid value when new
> thread is created.  However for our case we don't need to set it up,
> since the cur_mon variable is only used in such a pattern:
> 
>   old_mon = cur_mon;
>   cur_mon = xxx;
>   (do something, read cur_mon if necessary in the stack)
>   cur_mon = old_mon;
> 
> It plays a role as stack variable, so no need to be initialized at all.
> We only need to make sure the variable won't be changed unexpectedly by
> other threads.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> v3:
> - fix code style warning from patchew
> v2:
> - drop qemu-thread changes
> ---
>  include/monitor/monitor.h | 2 +-
>  monitor.c                 | 2 +-
>  stubs/monitor.c           | 2 +-
>  tests/test-util-sockets.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)

The Monitor object is not fully thread-safe, so although the correct
cur_mon is now accessible, code may still be unsafe.  For example,
monitor_get_fd(cur_mon, ...) is not thread-safe and must not be used by
OOB commands.

Future OOB commands need to know which monitor.h APIs are safe to call,
otherwise bugs are likely.  Please send a follow up patch to address
this (e.g. doc comments, locking where needed, etc).

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Peter Xu]

On Mon, Apr 16, 2018 at 04:37:48PM +0800, Stefan Hajnoczi wrote:
> On Thu, Apr 12, 2018 at 02:11:08PM +0800, Peter Xu wrote:
> > In the future the monitor iothread may be accessing the cur_mon as
> > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> > Out-Of-Band command, let's convert the cur_mon variable to be a
> > per-thread variable to make sure there won't be a race between threads.
> >
> > Note that thread variables are not initialized to a valid value when new
> > thread is created.  However for our case we don't need to set it up,
> > since the cur_mon variable is only used in such a pattern:
> > 
> >   old_mon = cur_mon;
> >   cur_mon = xxx;
> >   (do something, read cur_mon if necessary in the stack)
> >   cur_mon = old_mon;
> > 
> > It plays a role as stack variable, so no need to be initialized at all.
> > We only need to make sure the variable won't be changed unexpectedly by
> > other threads.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > v3:
> > - fix code style warning from patchew
> > v2:
> > - drop qemu-thread changes
> > ---
> >  include/monitor/monitor.h | 2 +-
> >  monitor.c                 | 2 +-
> >  stubs/monitor.c           | 2 +-
> >  tests/test-util-sockets.c | 2 +-
> >  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> The Monitor object is not fully thread-safe, so although the correct
> cur_mon is now accessible, code may still be unsafe.  For example,
> monitor_get_fd(cur_mon, ...) is not thread-safe and must not be used by
> OOB commands.

IMHO things like monitor_get_fd() should only be called in QMP
context, so there should always be a monitor_qmp_dispatch_one() in the
stack already (no matter whether it is in main thread or the monitor
iothread), which means that cur_mon should have been setup.  So IMHO
it's a programming error if monitor_get_fd() is called without correct
cur_mon setup after this patch.

> 
> Future OOB commands need to know which monitor.h APIs are safe to call,
> otherwise bugs are likely.  Please send a follow up patch to address
> this (e.g. doc comments, locking where needed, etc).
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks for reviewing,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]

On Mon, Apr 16, 2018 at 05:17:32PM +0800, Peter Xu wrote:
> On Mon, Apr 16, 2018 at 04:37:48PM +0800, Stefan Hajnoczi wrote:
> > On Thu, Apr 12, 2018 at 02:11:08PM +0800, Peter Xu wrote:
> > > In the future the monitor iothread may be accessing the cur_mon as
> > > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> > > Out-Of-Band command, let's convert the cur_mon variable to be a
> > > per-thread variable to make sure there won't be a race between threads.
> > >
> > > Note that thread variables are not initialized to a valid value when new
> > > thread is created.  However for our case we don't need to set it up,
> > > since the cur_mon variable is only used in such a pattern:
> > > 
> > >   old_mon = cur_mon;
> > >   cur_mon = xxx;
> > >   (do something, read cur_mon if necessary in the stack)
> > >   cur_mon = old_mon;
> > > 
> > > It plays a role as stack variable, so no need to be initialized at all.
> > > We only need to make sure the variable won't be changed unexpectedly by
> > > other threads.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > > v3:
> > > - fix code style warning from patchew
> > > v2:
> > > - drop qemu-thread changes
> > > ---
> > >  include/monitor/monitor.h | 2 +-
> > >  monitor.c                 | 2 +-
> > >  stubs/monitor.c           | 2 +-
> > >  tests/test-util-sockets.c | 2 +-
> > >  4 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > The Monitor object is not fully thread-safe, so although the correct
> > cur_mon is now accessible, code may still be unsafe.  For example,
> > monitor_get_fd(cur_mon, ...) is not thread-safe and must not be used by
> > OOB commands.
> 
> IMHO things like monitor_get_fd() should only be called in QMP
> context, so there should always be a monitor_qmp_dispatch_one() in the
> stack already (no matter whether it is in main thread or the monitor
> iothread), which means that cur_mon should have been setup.  So IMHO
> it's a programming error if monitor_get_fd() is called without correct
> cur_mon setup after this patch.

The pointer value of cur_mon is not the issue, you have made that work
correctly.  The problem is that some monitor.h APIs do not access the
Monitor object in a thread-safe fashion.

Two QMP commands executing simultaneously in the main loop thread and
the monitor IOThread can hit race conditions.  The example I gave was
the monitor_get_fd() API, which iterates and modifies the mon->fds
QLIST without a lock.

Please audit monitor.h and either make things thread-safe or document
the thread-safety rules (e.g. "This function cannot be called from
out-of-band QMP context").  This wasn't necessary before but now that
you are adding multi-threading it is.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Mon, Apr 16, 2018 at 05:17:32PM +0800, Peter Xu wrote:
>> On Mon, Apr 16, 2018 at 04:37:48PM +0800, Stefan Hajnoczi wrote:
>> > On Thu, Apr 12, 2018 at 02:11:08PM +0800, Peter Xu wrote:
>> > > In the future the monitor iothread may be accessing the cur_mon as
>> > > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
>> > > Out-Of-Band command, let's convert the cur_mon variable to be a
>> > > per-thread variable to make sure there won't be a race between threads.
>> > >
>> > > Note that thread variables are not initialized to a valid value when new
>> > > thread is created.  However for our case we don't need to set it up,
>> > > since the cur_mon variable is only used in such a pattern:
>> > > 
>> > >   old_mon = cur_mon;
>> > >   cur_mon = xxx;
>> > >   (do something, read cur_mon if necessary in the stack)
>> > >   cur_mon = old_mon;
>> > > 
>> > > It plays a role as stack variable, so no need to be initialized at all.
>> > > We only need to make sure the variable won't be changed unexpectedly by
>> > > other threads.
>> > > 
>> > > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > > ---
>> > > v3:
>> > > - fix code style warning from patchew
>> > > v2:
>> > > - drop qemu-thread changes
>> > > ---
>> > >  include/monitor/monitor.h | 2 +-
>> > >  monitor.c                 | 2 +-
>> > >  stubs/monitor.c           | 2 +-
>> > >  tests/test-util-sockets.c | 2 +-
>> > >  4 files changed, 4 insertions(+), 4 deletions(-)
>> > 
>> > The Monitor object is not fully thread-safe, so although the correct
>> > cur_mon is now accessible, code may still be unsafe.  For example,
>> > monitor_get_fd(cur_mon, ...) is not thread-safe and must not be used by
>> > OOB commands.
>> 
>> IMHO things like monitor_get_fd() should only be called in QMP
>> context, so there should always be a monitor_qmp_dispatch_one() in the
>> stack already (no matter whether it is in main thread or the monitor
>> iothread), which means that cur_mon should have been setup.  So IMHO
>> it's a programming error if monitor_get_fd() is called without correct
>> cur_mon setup after this patch.
>
> The pointer value of cur_mon is not the issue, you have made that work
> correctly.  The problem is that some monitor.h APIs do not access the
> Monitor object in a thread-safe fashion.
>
> Two QMP commands executing simultaneously in the main loop thread and
> the monitor IOThread can hit race conditions.  The example I gave was
> the monitor_get_fd() API, which iterates and modifies the mon->fds
> QLIST without a lock.
>
> Please audit monitor.h and either make things thread-safe or document
> the thread-safety rules (e.g. "This function cannot be called from
> out-of-band QMP context").  This wasn't necessary before but now that
> you are adding multi-threading it is.

Code working with the current thread's monitor via thread-local cur_mon
is easier to analyze in some ways than code working with a Monitor *
parameter: the latter can interfere with some other thread's monitor,
and you may have to argue what values the parameter can take.

You might want to replace parameters by cur_mon in certain cases.

Funnily, the plan used to be the opposite.  Commit 376253ece48: "On the
mid or long term, those use case will be obsoleted so that [cur_mon] can
be removed again."

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4074 bytes --]

On Tue, Apr 17, 2018 at 11:05:47AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > On Mon, Apr 16, 2018 at 05:17:32PM +0800, Peter Xu wrote:
> >> On Mon, Apr 16, 2018 at 04:37:48PM +0800, Stefan Hajnoczi wrote:
> >> > On Thu, Apr 12, 2018 at 02:11:08PM +0800, Peter Xu wrote:
> >> > > In the future the monitor iothread may be accessing the cur_mon as
> >> > > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> >> > > Out-Of-Band command, let's convert the cur_mon variable to be a
> >> > > per-thread variable to make sure there won't be a race between threads.
> >> > >
> >> > > Note that thread variables are not initialized to a valid value when new
> >> > > thread is created.  However for our case we don't need to set it up,
> >> > > since the cur_mon variable is only used in such a pattern:
> >> > > 
> >> > >   old_mon = cur_mon;
> >> > >   cur_mon = xxx;
> >> > >   (do something, read cur_mon if necessary in the stack)
> >> > >   cur_mon = old_mon;
> >> > > 
> >> > > It plays a role as stack variable, so no need to be initialized at all.
> >> > > We only need to make sure the variable won't be changed unexpectedly by
> >> > > other threads.
> >> > > 
> >> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> > > ---
> >> > > v3:
> >> > > - fix code style warning from patchew
> >> > > v2:
> >> > > - drop qemu-thread changes
> >> > > ---
> >> > >  include/monitor/monitor.h | 2 +-
> >> > >  monitor.c                 | 2 +-
> >> > >  stubs/monitor.c           | 2 +-
> >> > >  tests/test-util-sockets.c | 2 +-
> >> > >  4 files changed, 4 insertions(+), 4 deletions(-)
> >> > 
> >> > The Monitor object is not fully thread-safe, so although the correct
> >> > cur_mon is now accessible, code may still be unsafe.  For example,
> >> > monitor_get_fd(cur_mon, ...) is not thread-safe and must not be used by
> >> > OOB commands.
> >> 
> >> IMHO things like monitor_get_fd() should only be called in QMP
> >> context, so there should always be a monitor_qmp_dispatch_one() in the
> >> stack already (no matter whether it is in main thread or the monitor
> >> iothread), which means that cur_mon should have been setup.  So IMHO
> >> it's a programming error if monitor_get_fd() is called without correct
> >> cur_mon setup after this patch.
> >
> > The pointer value of cur_mon is not the issue, you have made that work
> > correctly.  The problem is that some monitor.h APIs do not access the
> > Monitor object in a thread-safe fashion.
> >
> > Two QMP commands executing simultaneously in the main loop thread and
> > the monitor IOThread can hit race conditions.  The example I gave was
> > the monitor_get_fd() API, which iterates and modifies the mon->fds
> > QLIST without a lock.
> >
> > Please audit monitor.h and either make things thread-safe or document
> > the thread-safety rules (e.g. "This function cannot be called from
> > out-of-band QMP context").  This wasn't necessary before but now that
> > you are adding multi-threading it is.
> 
> Code working with the current thread's monitor via thread-local cur_mon
> is easier to analyze in some ways than code working with a Monitor *
> parameter: the latter can interfere with some other thread's monitor,
> and you may have to argue what values the parameter can take.
> 
> You might want to replace parameters by cur_mon in certain cases.
> 
> Funnily, the plan used to be the opposite.  Commit 376253ece48: "On the
> mid or long term, those use case will be obsoleted so that [cur_mon] can
> be removed again."

Either way, the issue I described can still happen since two QMP
commands for a single Monitor object can execute simultaneously in the
main loop thread and the monitor IOThread.

I'm basically warning that QMP multi-threading isn't a solved problem
yet.  It needs to be solved by a combination of making things
thread-safe, documentation, and assertions so code fails loudly and
early when called from an unsupported context.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Peter Xu]

On Wed, Apr 18, 2018 at 01:12:24PM +0800, Stefan Hajnoczi wrote:
> On Tue, Apr 17, 2018 at 11:05:47AM +0200, Markus Armbruster wrote:
> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> > 
> > > On Mon, Apr 16, 2018 at 05:17:32PM +0800, Peter Xu wrote:
> > >> On Mon, Apr 16, 2018 at 04:37:48PM +0800, Stefan Hajnoczi wrote:
> > >> > On Thu, Apr 12, 2018 at 02:11:08PM +0800, Peter Xu wrote:
> > >> > > In the future the monitor iothread may be accessing the cur_mon as
> > >> > > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> > >> > > Out-Of-Band command, let's convert the cur_mon variable to be a
> > >> > > per-thread variable to make sure there won't be a race between threads.
> > >> > >
> > >> > > Note that thread variables are not initialized to a valid value when new
> > >> > > thread is created.  However for our case we don't need to set it up,
> > >> > > since the cur_mon variable is only used in such a pattern:
> > >> > > 
> > >> > >   old_mon = cur_mon;
> > >> > >   cur_mon = xxx;
> > >> > >   (do something, read cur_mon if necessary in the stack)
> > >> > >   cur_mon = old_mon;
> > >> > > 
> > >> > > It plays a role as stack variable, so no need to be initialized at all.
> > >> > > We only need to make sure the variable won't be changed unexpectedly by
> > >> > > other threads.
> > >> > > 
> > >> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > >> > > ---
> > >> > > v3:
> > >> > > - fix code style warning from patchew
> > >> > > v2:
> > >> > > - drop qemu-thread changes
> > >> > > ---
> > >> > >  include/monitor/monitor.h | 2 +-
> > >> > >  monitor.c                 | 2 +-
> > >> > >  stubs/monitor.c           | 2 +-
> > >> > >  tests/test-util-sockets.c | 2 +-
> > >> > >  4 files changed, 4 insertions(+), 4 deletions(-)
> > >> > 
> > >> > The Monitor object is not fully thread-safe, so although the correct
> > >> > cur_mon is now accessible, code may still be unsafe.  For example,
> > >> > monitor_get_fd(cur_mon, ...) is not thread-safe and must not be used by
> > >> > OOB commands.
> > >> 
> > >> IMHO things like monitor_get_fd() should only be called in QMP
> > >> context, so there should always be a monitor_qmp_dispatch_one() in the
> > >> stack already (no matter whether it is in main thread or the monitor
> > >> iothread), which means that cur_mon should have been setup.  So IMHO
> > >> it's a programming error if monitor_get_fd() is called without correct
> > >> cur_mon setup after this patch.
> > >
> > > The pointer value of cur_mon is not the issue, you have made that work
> > > correctly.  The problem is that some monitor.h APIs do not access the
> > > Monitor object in a thread-safe fashion.
> > >
> > > Two QMP commands executing simultaneously in the main loop thread and
> > > the monitor IOThread can hit race conditions.  The example I gave was
> > > the monitor_get_fd() API, which iterates and modifies the mon->fds
> > > QLIST without a lock.
> > >
> > > Please audit monitor.h and either make things thread-safe or document
> > > the thread-safety rules (e.g. "This function cannot be called from
> > > out-of-band QMP context").  This wasn't necessary before but now that
> > > you are adding multi-threading it is.
> > 
> > Code working with the current thread's monitor via thread-local cur_mon
> > is easier to analyze in some ways than code working with a Monitor *
> > parameter: the latter can interfere with some other thread's monitor,
> > and you may have to argue what values the parameter can take.
> > 
> > You might want to replace parameters by cur_mon in certain cases.
> > 
> > Funnily, the plan used to be the opposite.  Commit 376253ece48: "On the
> > mid or long term, those use case will be obsoleted so that [cur_mon] can
> > be removed again."

It's a pity that we didn't follow the plan!

> 
> Either way, the issue I described can still happen since two QMP
> commands for a single Monitor object can execute simultaneously in the
> main loop thread and the monitor IOThread.
> 
> I'm basically warning that QMP multi-threading isn't a solved problem
> yet.  It needs to be solved by a combination of making things
> thread-safe, documentation, and assertions so code fails loudly and
> early when called from an unsupported context.

I got your point now.  I'll went through monitor.h to see what I can
pick up besides the monitor_get_fd() related APIs.  I would prefer
locks comparing to documents if there aren't much work.

Thanks, Markus & Stefan.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Apr 18, 2018 at 01:12:24PM +0800, Stefan Hajnoczi wrote:
>> On Tue, Apr 17, 2018 at 11:05:47AM +0200, Markus Armbruster wrote:
>> > Stefan Hajnoczi <stefanha@redhat.com> writes:
>> > 
>> > > On Mon, Apr 16, 2018 at 05:17:32PM +0800, Peter Xu wrote:
>> > >> On Mon, Apr 16, 2018 at 04:37:48PM +0800, Stefan Hajnoczi wrote:
>> > >> > On Thu, Apr 12, 2018 at 02:11:08PM +0800, Peter Xu wrote:
>> > >> > > In the future the monitor iothread may be accessing the cur_mon as
>> > >> > > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
>> > >> > > Out-Of-Band command, let's convert the cur_mon variable to be a
>> > >> > > per-thread variable to make sure there won't be a race between threads.
>> > >> > >
>> > >> > > Note that thread variables are not initialized to a valid value when new
>> > >> > > thread is created.  However for our case we don't need to set it up,
>> > >> > > since the cur_mon variable is only used in such a pattern:
>> > >> > > 
>> > >> > >   old_mon = cur_mon;
>> > >> > >   cur_mon = xxx;
>> > >> > >   (do something, read cur_mon if necessary in the stack)
>> > >> > >   cur_mon = old_mon;
>> > >> > > 
>> > >> > > It plays a role as stack variable, so no need to be initialized at all.
>> > >> > > We only need to make sure the variable won't be changed unexpectedly by
>> > >> > > other threads.
>> > >> > > 
>> > >> > > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > >> > > ---
>> > >> > > v3:
>> > >> > > - fix code style warning from patchew
>> > >> > > v2:
>> > >> > > - drop qemu-thread changes
>> > >> > > ---
>> > >> > >  include/monitor/monitor.h | 2 +-
>> > >> > >  monitor.c                 | 2 +-
>> > >> > >  stubs/monitor.c           | 2 +-
>> > >> > >  tests/test-util-sockets.c | 2 +-
>> > >> > >  4 files changed, 4 insertions(+), 4 deletions(-)
>> > >> > 
>> > >> > The Monitor object is not fully thread-safe, so although the correct
>> > >> > cur_mon is now accessible, code may still be unsafe.  For example,
>> > >> > monitor_get_fd(cur_mon, ...) is not thread-safe and must not be used by
>> > >> > OOB commands.
>> > >> 
>> > >> IMHO things like monitor_get_fd() should only be called in QMP
>> > >> context, so there should always be a monitor_qmp_dispatch_one() in the
>> > >> stack already (no matter whether it is in main thread or the monitor
>> > >> iothread), which means that cur_mon should have been setup.  So IMHO
>> > >> it's a programming error if monitor_get_fd() is called without correct
>> > >> cur_mon setup after this patch.
>> > >
>> > > The pointer value of cur_mon is not the issue, you have made that work
>> > > correctly.  The problem is that some monitor.h APIs do not access the
>> > > Monitor object in a thread-safe fashion.
>> > >
>> > > Two QMP commands executing simultaneously in the main loop thread and
>> > > the monitor IOThread can hit race conditions.  The example I gave was
>> > > the monitor_get_fd() API, which iterates and modifies the mon->fds
>> > > QLIST without a lock.
>> > >
>> > > Please audit monitor.h and either make things thread-safe or document
>> > > the thread-safety rules (e.g. "This function cannot be called from
>> > > out-of-band QMP context").  This wasn't necessary before but now that
>> > > you are adding multi-threading it is.
>> > 
>> > Code working with the current thread's monitor via thread-local cur_mon
>> > is easier to analyze in some ways than code working with a Monitor *
>> > parameter: the latter can interfere with some other thread's monitor,
>> > and you may have to argue what values the parameter can take.
>> > 
>> > You might want to replace parameters by cur_mon in certain cases.
>> > 
>> > Funnily, the plan used to be the opposite.  Commit 376253ece48: "On the
>> > mid or long term, those use case will be obsoleted so that [cur_mon] can
>> > be removed again."
>
> It's a pity that we didn't follow the plan!

I don't think so.  Yes, global state is often a root of problems.  But
replacing global state by passing pointers around can hurt rather than
help with a more serious problem, namely *shared* state.

The less we pass around pointers to shared state, the easier we can
reason about how this state gets shared, and what needs to be done to
make that safe.

That's why I suggested to consider more direct use of the thread-local
Monitor *cur_mon.  Dereferencing cur_mon *obviously* uses this thread's
monitor.  Dereferencing a Monitor * parameter could use anything.

Use your judgement :)

[...]

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Peter Xu]

On Wed, Apr 18, 2018 at 09:45:27AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Apr 18, 2018 at 01:12:24PM +0800, Stefan Hajnoczi wrote:
> >> On Tue, Apr 17, 2018 at 11:05:47AM +0200, Markus Armbruster wrote:
> >> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> >> > 
> >> > > On Mon, Apr 16, 2018 at 05:17:32PM +0800, Peter Xu wrote:
> >> > >> On Mon, Apr 16, 2018 at 04:37:48PM +0800, Stefan Hajnoczi wrote:
> >> > >> > On Thu, Apr 12, 2018 at 02:11:08PM +0800, Peter Xu wrote:
> >> > >> > > In the future the monitor iothread may be accessing the cur_mon as
> >> > >> > > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> >> > >> > > Out-Of-Band command, let's convert the cur_mon variable to be a
> >> > >> > > per-thread variable to make sure there won't be a race between threads.
> >> > >> > >
> >> > >> > > Note that thread variables are not initialized to a valid value when new
> >> > >> > > thread is created.  However for our case we don't need to set it up,
> >> > >> > > since the cur_mon variable is only used in such a pattern:
> >> > >> > > 
> >> > >> > >   old_mon = cur_mon;
> >> > >> > >   cur_mon = xxx;
> >> > >> > >   (do something, read cur_mon if necessary in the stack)
> >> > >> > >   cur_mon = old_mon;
> >> > >> > > 
> >> > >> > > It plays a role as stack variable, so no need to be initialized at all.
> >> > >> > > We only need to make sure the variable won't be changed unexpectedly by
> >> > >> > > other threads.
> >> > >> > > 
> >> > >> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> > >> > > ---
> >> > >> > > v3:
> >> > >> > > - fix code style warning from patchew
> >> > >> > > v2:
> >> > >> > > - drop qemu-thread changes
> >> > >> > > ---
> >> > >> > >  include/monitor/monitor.h | 2 +-
> >> > >> > >  monitor.c                 | 2 +-
> >> > >> > >  stubs/monitor.c           | 2 +-
> >> > >> > >  tests/test-util-sockets.c | 2 +-
> >> > >> > >  4 files changed, 4 insertions(+), 4 deletions(-)
> >> > >> > 
> >> > >> > The Monitor object is not fully thread-safe, so although the correct
> >> > >> > cur_mon is now accessible, code may still be unsafe.  For example,
> >> > >> > monitor_get_fd(cur_mon, ...) is not thread-safe and must not be used by
> >> > >> > OOB commands.
> >> > >> 
> >> > >> IMHO things like monitor_get_fd() should only be called in QMP
> >> > >> context, so there should always be a monitor_qmp_dispatch_one() in the
> >> > >> stack already (no matter whether it is in main thread or the monitor
> >> > >> iothread), which means that cur_mon should have been setup.  So IMHO
> >> > >> it's a programming error if monitor_get_fd() is called without correct
> >> > >> cur_mon setup after this patch.
> >> > >
> >> > > The pointer value of cur_mon is not the issue, you have made that work
> >> > > correctly.  The problem is that some monitor.h APIs do not access the
> >> > > Monitor object in a thread-safe fashion.
> >> > >
> >> > > Two QMP commands executing simultaneously in the main loop thread and
> >> > > the monitor IOThread can hit race conditions.  The example I gave was
> >> > > the monitor_get_fd() API, which iterates and modifies the mon->fds
> >> > > QLIST without a lock.
> >> > >
> >> > > Please audit monitor.h and either make things thread-safe or document
> >> > > the thread-safety rules (e.g. "This function cannot be called from
> >> > > out-of-band QMP context").  This wasn't necessary before but now that
> >> > > you are adding multi-threading it is.
> >> > 
> >> > Code working with the current thread's monitor via thread-local cur_mon
> >> > is easier to analyze in some ways than code working with a Monitor *
> >> > parameter: the latter can interfere with some other thread's monitor,
> >> > and you may have to argue what values the parameter can take.
> >> > 
> >> > You might want to replace parameters by cur_mon in certain cases.
> >> > 
> >> > Funnily, the plan used to be the opposite.  Commit 376253ece48: "On the
> >> > mid or long term, those use case will be obsoleted so that [cur_mon] can
> >> > be removed again."
> >
> > It's a pity that we didn't follow the plan!
> 
> I don't think so.  Yes, global state is often a root of problems.  But
> replacing global state by passing pointers around can hurt rather than
> help with a more serious problem, namely *shared* state.
> 
> The less we pass around pointers to shared state, the easier we can
> reason about how this state gets shared, and what needs to be done to
> make that safe.
> 
> That's why I suggested to consider more direct use of the thread-local
> Monitor *cur_mon.  Dereferencing cur_mon *obviously* uses this thread's
> monitor.  Dereferencing a Monitor * parameter could use anything.

But note that when with Out-Of-Band cur_mon in thread A could be the
same cur_mon in thread B. E.g., when main thread is executing one
command for monitor M meanwhile the monitor iothread is running an
Out-Of-Band command for the same monitor M.  So AFAIU it looks just
like a Monitor* that passed in to the functions, the only difference
is that we pass that in by thread variables from the stack.  And I'm
afraid we'll face the same "shared state" problem too here...

> 
> Use your judgement :)

Will do! :)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Peter Xu]

On Thu, Apr 12, 2018 at 02:11:08PM +0800, Peter Xu wrote:
> In the future the monitor iothread may be accessing the cur_mon as
> well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> Out-Of-Band command, let's convert the cur_mon variable to be a
> per-thread variable to make sure there won't be a race between threads.
> 
> Note that thread variables are not initialized to a valid value when new
> thread is created.  However for our case we don't need to set it up,
> since the cur_mon variable is only used in such a pattern:
> 
>   old_mon = cur_mon;
>   cur_mon = xxx;
>   (do something, read cur_mon if necessary in the stack)
>   cur_mon = old_mon;
> 
> It plays a role as stack variable, so no need to be initialized at all.
> We only need to make sure the variable won't be changed unexpectedly by
> other threads.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> v3:
> - fix code style warning from patchew
> v2:
> - drop qemu-thread changes

Ping?

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> In the future the monitor iothread may be accessing the cur_mon as
> well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> Out-Of-Band command,

Uh, inhowfar are the commands marked allow-oob: true now not real?
These are migrate-recover, migrate-pause, x-oob-test.

Aside: having x-oob-test in QEMU proper is awful.  Is there really no
way around it?

>                      let's convert the cur_mon variable to be a
> per-thread variable to make sure there won't be a race between threads.
>
> Note that thread variables are not initialized to a valid value when new
> thread is created.  However for our case we don't need to set it up,
> since the cur_mon variable is only used in such a pattern:
>
>   old_mon = cur_mon;
>   cur_mon = xxx;
>   (do something, read cur_mon if necessary in the stack)
>   cur_mon = old_mon;
>
> It plays a role as stack variable, so no need to be initialized at all.
> We only need to make sure the variable won't be changed unexpectedly by
> other threads.

Do we plan to keep switching cur_mon forever?  Or do we intend to work
towards a 1:1 association between Monitor struct and monitor thread?

Even if we want the latter, I'm okay with this patch as an intermediate
step.

> Signed-off-by: Peter Xu <peterx@redhat.com>

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Peter Xu]

On Wed, May 23, 2018 at 10:23:22AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > In the future the monitor iothread may be accessing the cur_mon as
> > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> > Out-Of-Band command,
> 
> Uh, inhowfar are the commands marked allow-oob: true now not real?
> These are migrate-recover, migrate-pause, x-oob-test.

x-oob-test is unreal; the rest are real.

> 
> Aside: having x-oob-test in QEMU proper is awful.  Is there really no
> way around it?

I tried hard to think of a way but failed.  I need to let the
dispatcher stuck for a while, nothing can guarantee that but a lock. I
was using migration before to make sure dispatcher won't quite very
soon, but we have problems there since:

- some architectures may not even have RAM, so snapshot of those
  platforms can still be super fast;

- even if we make sure RAM is big, the time will depend on the speed
  of system, say, what if the test environment is running on RAM-disk
  which is still super fast even to dump the whole RAM onto disk?  As
  a conclusion - time based command won't work, because time is always
  a relative value, which can be affected by OS scheduler and system
  environments.

When there is better suggestion we can remove x-oob-test, but I can't
see any so far.

> 
> >                      let's convert the cur_mon variable to be a
> > per-thread variable to make sure there won't be a race between threads.
> >
> > Note that thread variables are not initialized to a valid value when new
> > thread is created.  However for our case we don't need to set it up,
> > since the cur_mon variable is only used in such a pattern:
> >
> >   old_mon = cur_mon;
> >   cur_mon = xxx;
> >   (do something, read cur_mon if necessary in the stack)
> >   cur_mon = old_mon;
> >
> > It plays a role as stack variable, so no need to be initialized at all.
> > We only need to make sure the variable won't be changed unexpectedly by
> > other threads.
> 
> Do we plan to keep switching cur_mon forever?  Or do we intend to work
> towards a 1:1 association between Monitor struct and monitor thread?

I still don't see a good way to remove the cur_mon switching... E.g.,
in qmp_human_monitor_command() we'll switch no matter what.

> 
> Even if we want the latter, I'm okay with this patch as an intermediate
> step.

That'll be appreciated.  Thanks,

> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, May 23, 2018 at 10:23:22AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > In the future the monitor iothread may be accessing the cur_mon as
>> > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
>> > Out-Of-Band command,
>> 
>> Uh, inhowfar are the commands marked allow-oob: true now not real?
>> These are migrate-recover, migrate-pause, x-oob-test.
>
> x-oob-test is unreal; the rest are real.

Sounds like the commit message needs tweaking then.

>> Aside: having x-oob-test in QEMU proper is awful.  Is there really no
>> way around it?
>
> I tried hard to think of a way but failed.  I need to let the
> dispatcher stuck for a while, nothing can guarantee that but a lock.

Paraphrasing, to make sure I understand:

0. We want to test an OOB command can "overtake" commands that hog the
   monitor.

1. We need a command to *reliably* take long enough for us to issue an
   OOB command.  Since OOB commands must not hog the monitor, this
   should be a non-OOB command.

2. We need an OOB command to do the overtaking.

3. We'd very much like to make the overaken command complete quickly
   after the OOB command overtook it, to keep the test quick.

Correct?

Your solution uses x-oob-test both for 1. and 2.  Problematic, because
x-oob-test is OOB, yet does something an OOB command must not do: block
on a lock.  Fixable.

>                                                                      I
> was using migration before to make sure dispatcher won't quite very
> soon, but we have problems there since:
>
> - some architectures may not even have RAM, so snapshot of those
>   platforms can still be super fast;
>
> - even if we make sure RAM is big, the time will depend on the speed
>   of system, say, what if the test environment is running on RAM-disk
>   which is still super fast even to dump the whole RAM onto disk?  As
>   a conclusion - time based command won't work, because time is always
>   a relative value, which can be affected by OS scheduler and system
>   environments.

Hmm, using migration is actually a cute idea.  But making it take long
enough (1.) is unreliable, as you found.  Also, we can't make it
complete quickly (3.).

> When there is better suggestion we can remove x-oob-test, but I can't
> see any so far.

The bit we actually need is having a command block on something we
control.  You picked a QemuSemaphore, internal to QEMU.  What about some
blocking system call?  Here's my idea:

0. Create a named pipe with mkfifo(1)

1. Issue some (non-OOB) QMP command that opens this pipe.  open()
   blocks.

2. Issue the OOB-command, verify it completes

3. If the non-OOB command writes to the pipe, suck the pipe dry, say
   with cat pipe | >/dev/null.  If it reads, give it something to read,
   say echo >pipe.

To make 3. complete quickly with a command that writes, you need one
that writes only a small amount of data.  Candidates: memsave, pmemsave,
screendump.

>> >                      let's convert the cur_mon variable to be a
>> > per-thread variable to make sure there won't be a race between threads.
>> >
>> > Note that thread variables are not initialized to a valid value when new
>> > thread is created.  However for our case we don't need to set it up,
>> > since the cur_mon variable is only used in such a pattern:
>> >
>> >   old_mon = cur_mon;
>> >   cur_mon = xxx;
>> >   (do something, read cur_mon if necessary in the stack)
>> >   cur_mon = old_mon;
>> >
>> > It plays a role as stack variable, so no need to be initialized at all.
>> > We only need to make sure the variable won't be changed unexpectedly by
>> > other threads.
>> 
>> Do we plan to keep switching cur_mon forever?  Or do we intend to work
>> towards a 1:1 association between Monitor struct and monitor thread?
>
> I still don't see a good way to remove the cur_mon switching... E.g.,
> in qmp_human_monitor_command() we'll switch no matter what.

That one's a hack.  We can make exceptions for hacks.  Where else do we
switch?

>> Even if we want the latter, I'm okay with this patch as an intermediate
>> step.
>
> That'll be appreciated.  Thanks,
>
>> 
>> > Signed-off-by: Peter Xu <peterx@redhat.com>

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Peter Xu]

On Wed, May 23, 2018 at 03:13:07PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, May 23, 2018 at 10:23:22AM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > In the future the monitor iothread may be accessing the cur_mon as
> >> > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> >> > Out-Of-Band command,
> >> 
> >> Uh, inhowfar are the commands marked allow-oob: true now not real?
> >> These are migrate-recover, migrate-pause, x-oob-test.
> >
> > x-oob-test is unreal; the rest are real.
> 
> Sounds like the commit message needs tweaking then.

Ah yes.  When I drafted this patch we don't really have the other two
commands yet.  They are there only after the postcopy recovery series.

Do you want me to repost one?

> 
> >> Aside: having x-oob-test in QEMU proper is awful.  Is there really no
> >> way around it?
> >
> > I tried hard to think of a way but failed.  I need to let the
> > dispatcher stuck for a while, nothing can guarantee that but a lock.
> 
> Paraphrasing, to make sure I understand:
> 
> 0. We want to test an OOB command can "overtake" commands that hog the
>    monitor.

Here to be more explicit - to test an OOB command can overtake
"non-OOB" commands that hog the monitor.

Note that one OOB command still cannot overtake another OOB command
now.  Since we are sharing the same thread for all the monitors, even
we can't let monitor A's OOB command overtake another OOB command of
monitor B.  After all, all OOB commands are run in the parser, and now
QMP only has one thread to do the parsing.

> 
> 1. We need a command to *reliably* take long enough for us to issue an
>    OOB command.  Since OOB commands must not hog the monitor, this
>    should be a non-OOB command.
> 
> 2. We need an OOB command to do the overtaking.
> 
> 3. We'd very much like to make the overaken command complete quickly
>    after the OOB command overtook it, to keep the test quick.
> 
> Correct?

For the rest, I think yes.

> 
> Your solution uses x-oob-test both for 1. and 2.  Problematic, because
> x-oob-test is OOB, yet does something an OOB command must not do: block
> on a lock.  Fixable.

Yes if to be strict it is.  :)

Before all the rest, I want to make sure we have the same
understanding on OOB: when we say "one command is an OOB command" it
does not mean it'll always be executed in OOB fasion.  Let's take
x-oob-test for example: although it is an OOB command, it means that
it can be run out of band, however it can also be run in band too,
which depends on whether we have "control": { "run-oob": true } in the
"execute" command.  Actually in our OOB test the locking operation is
done in band, only the unlocking is done out of band.  Quote from
function test_qmp_oob():

    qtest_async_qmp(qts,
                    "{ 'execute': 'x-oob-test',"
                    "  'arguments': { 'lock': true }, "
                    "  'id': 'lock-cmd'}");
    qtest_async_qmp(qts,
                    "{ 'execute': 'x-oob-test', "
                    "  'arguments': { 'lock': false }, "
                    "  'control': { 'run-oob': true }, "
                    "  'id': 'unlock-cmd' }");

If we want to be strict on this, we can split x-oob-test into:

- x-oob-test-lock: this can be a non-OOB command
- x-oob-test-unlock: this must be an OOB command

Then the second x-oob-test-unlock will satisfy our "strict
requirement" on OOB command.

Here though I would still prefer to keep the x-oob-test command,
because the strict rules are for people who are not familiar with OOB.
For us, we are very sure that it will work for us (or say, we should
never call the lock operation with OOB context).  It might be an
overkill to introduce two test commands for this.

> 
> >                                                                      I
> > was using migration before to make sure dispatcher won't quite very
> > soon, but we have problems there since:
> >
> > - some architectures may not even have RAM, so snapshot of those
> >   platforms can still be super fast;
> >
> > - even if we make sure RAM is big, the time will depend on the speed
> >   of system, say, what if the test environment is running on RAM-disk
> >   which is still super fast even to dump the whole RAM onto disk?  As
> >   a conclusion - time based command won't work, because time is always
> >   a relative value, which can be affected by OS scheduler and system
> >   environments.
> 
> Hmm, using migration is actually a cute idea.  But making it take long
> enough (1.) is unreliable, as you found.  Also, we can't make it
> complete quickly (3.).

Agreed.

> 
> > When there is better suggestion we can remove x-oob-test, but I can't
> > see any so far.
> 
> The bit we actually need is having a command block on something we
> control.  You picked a QemuSemaphore, internal to QEMU.  What about some
> blocking system call?  Here's my idea:
> 
> 0. Create a named pipe with mkfifo(1)
> 
> 1. Issue some (non-OOB) QMP command that opens this pipe.  open()
>    blocks.
> 
> 2. Issue the OOB-command, verify it completes
> 
> 3. If the non-OOB command writes to the pipe, suck the pipe dry, say
>    with cat pipe | >/dev/null.  If it reads, give it something to read,
>    say echo >pipe.
> 
> To make 3. complete quickly with a command that writes, you need one
> that writes only a small amount of data.  Candidates: memsave, pmemsave,
> screendump.

I am not sure I have fully understood above, please correct me if I
haven't.

IMHO they are just similar ideas just like when we use semaphores.
Semaphores are QEMU internal, but underneath that it'll also do
syscalls (I believe we are using futex() syscalls for semaphores,
though that should be in libc code not QEMU).  I believe fifos can be
as easy to implement as semaphores, but I don't see much difference
here - IMHO fifo just use its own way to do similar thing.  On the
kernel side, both of them will basically do:

(1) schedule() self process out to wait for the event
(2) wake_up() by another process

>From that point of view, they are same to me.

In other words, since we don't allow OOB-capable command handlers to
take risky locks, meanwhile we should also expand that idea further
into any operation that might block for some time.  Reading a fifo
here is the same as taking a risky lock - we need to make sure the
fifo read won't block for long or we should never do that in OOB
command handlers.

> 
> >> >                      let's convert the cur_mon variable to be a
> >> > per-thread variable to make sure there won't be a race between threads.
> >> >
> >> > Note that thread variables are not initialized to a valid value when new
> >> > thread is created.  However for our case we don't need to set it up,
> >> > since the cur_mon variable is only used in such a pattern:
> >> >
> >> >   old_mon = cur_mon;
> >> >   cur_mon = xxx;
> >> >   (do something, read cur_mon if necessary in the stack)
> >> >   cur_mon = old_mon;
> >> >
> >> > It plays a role as stack variable, so no need to be initialized at all.
> >> > We only need to make sure the variable won't be changed unexpectedly by
> >> > other threads.
> >> 
> >> Do we plan to keep switching cur_mon forever?  Or do we intend to work
> >> towards a 1:1 association between Monitor struct and monitor thread?
> >
> > I still don't see a good way to remove the cur_mon switching... E.g.,
> > in qmp_human_monitor_command() we'll switch no matter what.
> 
> That one's a hack.  We can make exceptions for hacks.  Where else do we
> switch?

Yeah, the major one should be monitor_qmp_dispatch_one().  If we can
have one thread per monitor, it seems to be good we can at least omit
the switching for this one.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, May 23, 2018 at 03:13:07PM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Wed, May 23, 2018 at 10:23:22AM +0200, Markus Armbruster wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > In the future the monitor iothread may be accessing the cur_mon as
>> >> > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
>> >> > Out-Of-Band command,
>> >> 
>> >> Uh, inhowfar are the commands marked allow-oob: true now not real?
>> >> These are migrate-recover, migrate-pause, x-oob-test.
>> >
>> > x-oob-test is unreal; the rest are real.
>> 
>> Sounds like the commit message needs tweaking then.
>
> Ah yes.  When I drafted this patch we don't really have the other two
> commands yet.  They are there only after the postcopy recovery series.
>
> Do you want me to repost one?

Repost is fine, but I'm also happy to edit commit messages when I apply
to qapi-next.

>> >> Aside: having x-oob-test in QEMU proper is awful.  Is there really no
>> >> way around it?
>> >
>> > I tried hard to think of a way but failed.  I need to let the
>> > dispatcher stuck for a while, nothing can guarantee that but a lock.
>> 
>> Paraphrasing, to make sure I understand:
>> 
>> 0. We want to test an OOB command can "overtake" commands that hog the
>>    monitor.
>
> Here to be more explicit - to test an OOB command can overtake
> "non-OOB" commands that hog the monitor.
>
> Note that one OOB command still cannot overtake another OOB command
> now.  Since we are sharing the same thread for all the monitors, even
> we can't let monitor A's OOB command overtake another OOB command of
> monitor B.  After all, all OOB commands are run in the parser, and now
> QMP only has one thread to do the parsing.

Understood.

OOB commands must complete quicky.  Their code is effectively realtime
code.

>> 
>> 1. We need a command to *reliably* take long enough for us to issue an
>>    OOB command.  Since OOB commands must not hog the monitor, this
>>    should be a non-OOB command.
>> 
>> 2. We need an OOB command to do the overtaking.
>> 
>> 3. We'd very much like to make the overaken command complete quickly
>>    after the OOB command overtook it, to keep the test quick.
>> 
>> Correct?
>
> For the rest, I think yes.

Good :)

>> Your solution uses x-oob-test both for 1. and 2.  Problematic, because
>> x-oob-test is OOB, yet does something an OOB command must not do: block
>> on a lock.  Fixable.
>
> Yes if to be strict it is.  :)
>
> Before all the rest, I want to make sure we have the same
> understanding on OOB: when we say "one command is an OOB command" it
> does not mean it'll always be executed in OOB fasion.  Let's take
> x-oob-test for example: although it is an OOB command, it means that
> it can be run out of band, however it can also be run in band too,
> which depends on whether we have "control": { "run-oob": true } in the
> "execute" command.  Actually in our OOB test the locking operation is
> done in band, only the unlocking is done out of band.  Quote from
> function test_qmp_oob():
>
>     qtest_async_qmp(qts,
>                     "{ 'execute': 'x-oob-test',"
>                     "  'arguments': { 'lock': true }, "
>                     "  'id': 'lock-cmd'}");
>     qtest_async_qmp(qts,
>                     "{ 'execute': 'x-oob-test', "
>                     "  'arguments': { 'lock': false }, "
>                     "  'control': { 'run-oob': true }, "
>                     "  'id': 'unlock-cmd' }");

I missed that detail.  There is nothing to fix, actually.

> If we want to be strict on this, we can split x-oob-test into:
>
> - x-oob-test-lock: this can be a non-OOB command
> - x-oob-test-unlock: this must be an OOB command
>
> Then the second x-oob-test-unlock will satisfy our "strict
> requirement" on OOB command.

Not necessary, your hack is fine as is.

Having a real OOB command that must not be executed with run-oob: true
in some cases would not be fine.

> Here though I would still prefer to keep the x-oob-test command,
> because the strict rules are for people who are not familiar with OOB.
> For us, we are very sure that it will work for us (or say, we should
> never call the lock operation with OOB context).  It might be an
> overkill to introduce two test commands for this.

Agreed.

>> > was using migration before to make sure dispatcher won't quite very
>> > soon, but we have problems there since:
>> >
>> > - some architectures may not even have RAM, so snapshot of those
>> >   platforms can still be super fast;
>> >
>> > - even if we make sure RAM is big, the time will depend on the speed
>> >   of system, say, what if the test environment is running on RAM-disk
>> >   which is still super fast even to dump the whole RAM onto disk?  As
>> >   a conclusion - time based command won't work, because time is always
>> >   a relative value, which can be affected by OS scheduler and system
>> >   environments.
>> 
>> Hmm, using migration is actually a cute idea.  But making it take long
>> enough (1.) is unreliable, as you found.  Also, we can't make it
>> complete quickly (3.).
>
> Agreed.
>
>> 
>> > When there is better suggestion we can remove x-oob-test, but I can't
>> > see any so far.
>> 
>> The bit we actually need is having a command block on something we
>> control.  You picked a QemuSemaphore, internal to QEMU.  What about some
>> blocking system call?  Here's my idea:
>> 
>> 0. Create a named pipe with mkfifo(1)
>> 
>> 1. Issue some (non-OOB) QMP command that opens this pipe.  open()
>>    blocks.
>> 
>> 2. Issue the OOB-command, verify it completes
>> 
>> 3. If the non-OOB command writes to the pipe, suck the pipe dry, say
>>    with cat pipe | >/dev/null.  If it reads, give it something to read,
>>    say echo >pipe.
>> 
>> To make 3. complete quickly with a command that writes, you need one
>> that writes only a small amount of data.  Candidates: memsave, pmemsave,
>> screendump.
>
> I am not sure I have fully understood above, please correct me if I
> haven't.
>
> IMHO they are just similar ideas just like when we use semaphores.
> Semaphores are QEMU internal, but underneath that it'll also do
> syscalls (I believe we are using futex() syscalls for semaphores,
> though that should be in libc code not QEMU).  I believe fifos can be
> as easy to implement as semaphores, but I don't see much difference
> here - IMHO fifo just use its own way to do similar thing.  On the
> kernel side, both of them will basically do:
>
> (1) schedule() self process out to wait for the event
> (2) wake_up() by another process
>
> From that point of view, they are same to me.

The difference is we don't need a special command just for the test.

> In other words, since we don't allow OOB-capable command handlers to
> take risky locks, meanwhile we should also expand that idea further
> into any operation that might block for some time.  Reading a fifo
> here is the same as taking a risky lock - we need to make sure the
> fifo read won't block for long or we should never do that in OOB
> command handlers.

Absolutely.  Any code that can run in an OOB command is realtime code,
and that means it must not block for an indeterminate time.  Synchronous
read() is right out.

>> >> >                      let's convert the cur_mon variable to be a
>> >> > per-thread variable to make sure there won't be a race between threads.
>> >> >
>> >> > Note that thread variables are not initialized to a valid value when new
>> >> > thread is created.  However for our case we don't need to set it up,
>> >> > since the cur_mon variable is only used in such a pattern:
>> >> >
>> >> >   old_mon = cur_mon;
>> >> >   cur_mon = xxx;
>> >> >   (do something, read cur_mon if necessary in the stack)
>> >> >   cur_mon = old_mon;
>> >> >
>> >> > It plays a role as stack variable, so no need to be initialized at all.
>> >> > We only need to make sure the variable won't be changed unexpectedly by
>> >> > other threads.
>> >> 
>> >> Do we plan to keep switching cur_mon forever?  Or do we intend to work
>> >> towards a 1:1 association between Monitor struct and monitor thread?
>> >
>> > I still don't see a good way to remove the cur_mon switching... E.g.,
>> > in qmp_human_monitor_command() we'll switch no matter what.
>> 
>> That one's a hack.  We can make exceptions for hacks.  Where else do we
>> switch?
>
> Yeah, the major one should be monitor_qmp_dispatch_one().  If we can
> have one thread per monitor, it seems to be good we can at least omit
> the switching for this one.

Yes, please.

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Peter Xu]

On Thu, May 24, 2018 at 10:22:48AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, May 23, 2018 at 03:13:07PM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Wed, May 23, 2018 at 10:23:22AM +0200, Markus Armbruster wrote:
> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >> 
> >> >> > In the future the monitor iothread may be accessing the cur_mon as
> >> >> > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> >> >> > Out-Of-Band command,
> >> >> 
> >> >> Uh, inhowfar are the commands marked allow-oob: true now not real?
> >> >> These are migrate-recover, migrate-pause, x-oob-test.
> >> >
> >> > x-oob-test is unreal; the rest are real.
> >> 
> >> Sounds like the commit message needs tweaking then.
> >
> > Ah yes.  When I drafted this patch we don't really have the other two
> > commands yet.  They are there only after the postcopy recovery series.
> >
> > Do you want me to repost one?
> 
> Repost is fine, but I'm also happy to edit commit messages when I apply
> to qapi-next.

If this is the only thing to touch up, I would appreciate if you can
do that to avoid message bounces.

[...]

> >> > When there is better suggestion we can remove x-oob-test, but I can't
> >> > see any so far.
> >> 
> >> The bit we actually need is having a command block on something we
> >> control.  You picked a QemuSemaphore, internal to QEMU.  What about some
> >> blocking system call?  Here's my idea:
> >> 
> >> 0. Create a named pipe with mkfifo(1)
> >> 
> >> 1. Issue some (non-OOB) QMP command that opens this pipe.  open()
> >>    blocks.
> >> 
> >> 2. Issue the OOB-command, verify it completes
> >> 
> >> 3. If the non-OOB command writes to the pipe, suck the pipe dry, say
> >>    with cat pipe | >/dev/null.  If it reads, give it something to read,
> >>    say echo >pipe.
> >> 
> >> To make 3. complete quickly with a command that writes, you need one
> >> that writes only a small amount of data.  Candidates: memsave, pmemsave,
> >> screendump.
> >
> > I am not sure I have fully understood above, please correct me if I
> > haven't.
> >
> > IMHO they are just similar ideas just like when we use semaphores.
> > Semaphores are QEMU internal, but underneath that it'll also do
> > syscalls (I believe we are using futex() syscalls for semaphores,
> > though that should be in libc code not QEMU).  I believe fifos can be
> > as easy to implement as semaphores, but I don't see much difference
> > here - IMHO fifo just use its own way to do similar thing.  On the
> > kernel side, both of them will basically do:
> >
> > (1) schedule() self process out to wait for the event
> > (2) wake_up() by another process
> >
> > From that point of view, they are same to me.
> 
> The difference is we don't need a special command just for the test.

Ah I see the point now - an existing command that will open a fifo.
Yes that works too, though we'll need to find a good candidate, add
some work to prepare the testbed and make sure there's no side effect.
Anyway, I am totally fine with it once we really want it that way.

> 
> > In other words, since we don't allow OOB-capable command handlers to
> > take risky locks, meanwhile we should also expand that idea further
> > into any operation that might block for some time.  Reading a fifo
> > here is the same as taking a risky lock - we need to make sure the
> > fifo read won't block for long or we should never do that in OOB
> > command handlers.
> 
> Absolutely.  Any code that can run in an OOB command is realtime code,
> and that means it must not block for an indeterminate time.  Synchronous
> read() is right out.
> 
> >> >> >                      let's convert the cur_mon variable to be a
> >> >> > per-thread variable to make sure there won't be a race between threads.
> >> >> >
> >> >> > Note that thread variables are not initialized to a valid value when new
> >> >> > thread is created.  However for our case we don't need to set it up,
> >> >> > since the cur_mon variable is only used in such a pattern:
> >> >> >
> >> >> >   old_mon = cur_mon;
> >> >> >   cur_mon = xxx;
> >> >> >   (do something, read cur_mon if necessary in the stack)
> >> >> >   cur_mon = old_mon;
> >> >> >
> >> >> > It plays a role as stack variable, so no need to be initialized at all.
> >> >> > We only need to make sure the variable won't be changed unexpectedly by
> >> >> > other threads.
> >> >> 
> >> >> Do we plan to keep switching cur_mon forever?  Or do we intend to work
> >> >> towards a 1:1 association between Monitor struct and monitor thread?
> >> >
> >> > I still don't see a good way to remove the cur_mon switching... E.g.,
> >> > in qmp_human_monitor_command() we'll switch no matter what.
> >> 
> >> That one's a hack.  We can make exceptions for hacks.  Where else do we
> >> switch?
> >
> > Yeah, the major one should be monitor_qmp_dispatch_one().  If we can
> > have one thread per monitor, it seems to be good we can at least omit
> > the switching for this one.
> 
> Yes, please.

Note that currently it's still not 1:1 - we only have one thread for
all the monitors, so now it seems that we still cannot omit it.  And
I'm not sure whether we should do that only to omit this cur_mon
fiddling, since it might be an overkill too.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3] monitor: let cur_mon be per-thread

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 24, 2018 at 10:22:48AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Wed, May 23, 2018 at 03:13:07PM +0200, Markus Armbruster wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > On Wed, May 23, 2018 at 10:23:22AM +0200, Markus Armbruster wrote:
>> >> >> Peter Xu <peterx@redhat.com> writes:
>> >> >> 
>> >> >> > In the future the monitor iothread may be accessing the cur_mon as
>> >> >> > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
>> >> >> > Out-Of-Band command,
>> >> >> 
>> >> >> Uh, inhowfar are the commands marked allow-oob: true now not real?
>> >> >> These are migrate-recover, migrate-pause, x-oob-test.
>> >> >
>> >> > x-oob-test is unreal; the rest are real.
>> >> 
>> >> Sounds like the commit message needs tweaking then.
>> >
>> > Ah yes.  When I drafted this patch we don't really have the other two
>> > commands yet.  They are there only after the postcopy recovery series.
>> >
>> > Do you want me to repost one?
>> 
>> Repost is fine, but I'm also happy to edit commit messages when I apply
>> to qapi-next.
>
> If this is the only thing to touch up, I would appreciate if you can
> do that to avoid message bounces.

Okay.

> [...]
>
>> >> > When there is better suggestion we can remove x-oob-test, but I can't
>> >> > see any so far.
>> >> 
>> >> The bit we actually need is having a command block on something we
>> >> control.  You picked a QemuSemaphore, internal to QEMU.  What about some
>> >> blocking system call?  Here's my idea:
>> >> 
>> >> 0. Create a named pipe with mkfifo(1)
>> >> 
>> >> 1. Issue some (non-OOB) QMP command that opens this pipe.  open()
>> >>    blocks.
>> >> 
>> >> 2. Issue the OOB-command, verify it completes
>> >> 
>> >> 3. If the non-OOB command writes to the pipe, suck the pipe dry, say
>> >>    with cat pipe | >/dev/null.  If it reads, give it something to read,
>> >>    say echo >pipe.
>> >> 
>> >> To make 3. complete quickly with a command that writes, you need one
>> >> that writes only a small amount of data.  Candidates: memsave, pmemsave,
>> >> screendump.
>> >
>> > I am not sure I have fully understood above, please correct me if I
>> > haven't.
>> >
>> > IMHO they are just similar ideas just like when we use semaphores.
>> > Semaphores are QEMU internal, but underneath that it'll also do
>> > syscalls (I believe we are using futex() syscalls for semaphores,
>> > though that should be in libc code not QEMU).  I believe fifos can be
>> > as easy to implement as semaphores, but I don't see much difference
>> > here - IMHO fifo just use its own way to do similar thing.  On the
>> > kernel side, both of them will basically do:
>> >
>> > (1) schedule() self process out to wait for the event
>> > (2) wake_up() by another process
>> >
>> > From that point of view, they are same to me.
>> 
>> The difference is we don't need a special command just for the test.
>
> Ah I see the point now - an existing command that will open a fifo.
> Yes that works too, though we'll need to find a good candidate, add
> some work to prepare the testbed and make sure there's no side effect.
> Anyway, I am totally fine with it once we really want it that way.

Separate patch, not blocking this series.

>> > In other words, since we don't allow OOB-capable command handlers to
>> > take risky locks, meanwhile we should also expand that idea further
>> > into any operation that might block for some time.  Reading a fifo
>> > here is the same as taking a risky lock - we need to make sure the
>> > fifo read won't block for long or we should never do that in OOB
>> > command handlers.
>> 
>> Absolutely.  Any code that can run in an OOB command is realtime code,
>> and that means it must not block for an indeterminate time.  Synchronous
>> read() is right out.
>> 
>> >> >> >                      let's convert the cur_mon variable to be a
>> >> >> > per-thread variable to make sure there won't be a race between threads.
>> >> >> >
>> >> >> > Note that thread variables are not initialized to a valid value when new
>> >> >> > thread is created.  However for our case we don't need to set it up,
>> >> >> > since the cur_mon variable is only used in such a pattern:
>> >> >> >
>> >> >> >   old_mon = cur_mon;
>> >> >> >   cur_mon = xxx;
>> >> >> >   (do something, read cur_mon if necessary in the stack)
>> >> >> >   cur_mon = old_mon;
>> >> >> >
>> >> >> > It plays a role as stack variable, so no need to be initialized at all.
>> >> >> > We only need to make sure the variable won't be changed unexpectedly by
>> >> >> > other threads.
>> >> >> 
>> >> >> Do we plan to keep switching cur_mon forever?  Or do we intend to work
>> >> >> towards a 1:1 association between Monitor struct and monitor thread?
>> >> >
>> >> > I still don't see a good way to remove the cur_mon switching... E.g.,
>> >> > in qmp_human_monitor_command() we'll switch no matter what.
>> >> 
>> >> That one's a hack.  We can make exceptions for hacks.  Where else do we
>> >> switch?
>> >
>> > Yeah, the major one should be monitor_qmp_dispatch_one().  If we can
>> > have one thread per monitor, it seems to be good we can at least omit
>> > the switching for this one.
>> 
>> Yes, please.
>
> Note that currently it's still not 1:1 - we only have one thread for
> all the monitors, so now it seems that we still cannot omit it.  And
> I'm not sure whether we should do that only to omit this cur_mon
> fiddling, since it might be an overkill too.

If we decide we want 1:1, we should make 1:1 as obvious as we can in its
implementation.  That means getting rid of cur_mon switching.  Anyway,
not an immediate worry.