Hi,

The chardev context switching code is a bit fragile, as it works as if
the current context is properly synchronized with the new context.  It
isn't so obvious to me that concurrent usage of chardev can't happen,
as there might be various main loop sources being dispatched during
the switch.

Worried about the situation, I wrote those patches a while ago, I
think they are still worth to consider. I used to have some basic
test, but it now conflicts a lot with recent changes. I would like to
get some feedback about the series before I rewrite it.

The importat patch is "chardev: make qemu_chr_fe_set_handlers()
context switching safer". It works by "freezing" the given contexts
while the chardev will "move" (recreate to the new context) the
various sources it owns. This looks quite ugly to me overall, but still
safer than today.

This should allow to simplify the scary code from "monitor: set the
chardev context from the main context/thread".

Finally, "char-socket: restart the reconnect timer to switch context"
shows that we have chardev backends that do not switch fully yet.

(there should be a meme "using chardev from multiple threads is
considered harmful")

Marc-André Lureau (4):
  iothread: wait until the glib context is acquired
  chardev: make qemu_chr_fe_set_handlers() context switching safer
  monitor: set the chardev context from the main context/thread
  char-socket: restart the reconnect timer to switch context

 include/chardev/char-fe.h |  23 +++++++++
 chardev/char-fe.c         | 103 +++++++++++++++++++++++++++++++++-----
 chardev/char-mux.c        |  14 +++---
 chardev/char-socket.c     |   5 ++
 iothread.c                |   7 +++
 monitor.c                 |  39 +++------------
 6 files changed, 139 insertions(+), 52 deletions(-)

-- 
2.21.0.rc1

On Wed, Feb 20, 2019 at 05:06:24PM +0100, Marc-André Lureau wrote:
> Hi,

Hi,

> 
> The chardev context switching code is a bit fragile, as it works as if
> the current context is properly synchronized with the new context.  It
> isn't so obvious to me that concurrent usage of chardev can't happen,
> as there might be various main loop sources being dispatched during
> the switch.
> 
> Worried about the situation, I wrote those patches a while ago, I
> think they are still worth to consider. I used to have some basic
> test, but it now conflicts a lot with recent changes. I would like to
> get some feedback about the series before I rewrite it.
> 
> The importat patch is "chardev: make qemu_chr_fe_set_handlers()
> context switching safer". It works by "freezing" the given contexts
> while the chardev will "move" (recreate to the new context) the
> various sources it owns. This looks quite ugly to me overall, but still
> safer than today.
> 
> This should allow to simplify the scary code from "monitor: set the
> chardev context from the main context/thread".

Indeed it's at least not that friendly to readers... so out of
curiosity - is this the only reason for this series?

> 
> Finally, "char-socket: restart the reconnect timer to switch context"
> shows that we have chardev backends that do not switch fully yet.

This seems irrelevant to the series, or am I wrong?

Thanks,

-- 
Peter Xu

Hi

On Thu, Feb 21, 2019 at 8:58 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Feb 20, 2019 at 05:06:24PM +0100, Marc-André Lureau wrote:
> > Hi,
>
> Hi,
>
> >
> > The chardev context switching code is a bit fragile, as it works as if
> > the current context is properly synchronized with the new context.  It
> > isn't so obvious to me that concurrent usage of chardev can't happen,
> > as there might be various main loop sources being dispatched during
> > the switch.
> >
> > Worried about the situation, I wrote those patches a while ago, I
> > think they are still worth to consider. I used to have some basic
> > test, but it now conflicts a lot with recent changes. I would like to
> > get some feedback about the series before I rewrite it.
> >
> > The importat patch is "chardev: make qemu_chr_fe_set_handlers()
> > context switching safer". It works by "freezing" the given contexts
> > while the chardev will "move" (recreate to the new context) the
> > various sources it owns. This looks quite ugly to me overall, but still
> > safer than today.
> >
> > This should allow to simplify the scary code from "monitor: set the
> > chardev context from the main context/thread".
>
> Indeed it's at least not that friendly to readers... so out of
> curiosity - is this the only reason for this series?

One of the reasons. The main reason is that chardev are *not*
thread-safe, yet we are treating them like if it would be fine to
manipulate from different threads (this oneshot code in the monitor is
one example indeed).

Since the sources are attached to different contexts, if we freeze the
old/new contexts, we have *some* guarantee that concurrent usage will
be a bit more limited...


>
> >
> > Finally, "char-socket: restart the reconnect timer to switch context"
> > shows that we have chardev backends that do not switch fully yet.
>
> This seems irrelevant to the series, or am I wrong?

Kinda related, even with the context freeze proposed here, there may
be concurrent access because we forgot some sources, or other threads
etc.

thanks