Re: [Qemu-devel] [PATCH 2/4] chardev: make qemu_chr_fe_set_handlers() context switching safer

[Marc-André Lureau]

Hi

Le ven. 22 févr. 2019 à 09:33, Philippe Mathieu-Daudé <address@hidden> a
écrit :

>
>
> On 2/21/19 9:03 AM, Peter Xu wrote:
> > On Wed, Feb 20, 2019 at 05:06:26PM +0100, Marc-André Lureau wrote:
> >> qemu_chr_fe_set_handlers() may switch the context of various
> >> sources. In order to prevent dispatch races from different threads,
> >> let's acquire or freeze the context, do all the source switches, and
> >> then release/resume the contexts. This should help to make context
> >> switching safer.
> >>
> >> Signed-off-by: Marc-André Lureau <address@hidden>
> >> ---
> >>  include/chardev/char-fe.h |  23 +++++++++
> >>  chardev/char-fe.c         | 103 +++++++++++++++++++++++++++++++++-----
> >>  chardev/char-mux.c        |  14 +++---
> >>  3 files changed, 121 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> >> index aa1b864ccd..4051435a1c 100644
> >> --- a/include/chardev/char-fe.h
> >> +++ b/include/chardev/char-fe.h
> >> @@ -84,6 +84,14 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
> >>   * Set the front end char handlers. The front end takes the focus if
> >>   * any of the handler is non-NULL.
> >>   *
> >> + * A chardev may have multiple main loop sources. In order to prevent
> >> + * races when switching contexts, the function will temporarily block
> >> + * the contexts before the source switch to prevent them from
> >> + * dispatching in different threads concurrently.
> >> + *
> >> + * The current and the new @context must be acquirable or
> >> + * running & dispatched in a loop (the function will hang otherwise).
> >> + *
> >>   * Without associated Chardev, nothing is changed.
> >>   */
> >>  void qemu_chr_fe_set_handlers_full(CharBackend *b,
> >> @@ -110,6 +118,21 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
> >>                                GMainContext *context,
> >>                                bool set_open);
> >>
> >> +/**
> >> + * qemu_chr_fe_set_handlers_internal:
> >> + *
> >> + * Same as qemu_chr_fe_set_handlers(), without context freezing.
> >> + */
> >> +void qemu_chr_fe_set_handlers_internal(CharBackend *b,
> >> +                                       IOCanReadHandler *fd_can_read,
> >> +                                       IOReadHandler *fd_read,
> >> +                                       IOEventHandler *fd_event,
> >> +                                       BackendChangeHandler *be_change,
> >> +                                       void *opaque,
> >> +                                       GMainContext *context,
> >> +                                       bool set_open,
> >> +                                       bool sync_state);
>
> Can we add this function into a new header "chardev/char-internal.h"
> (internal to chardev/) rather than "include/chardev/char-fe.h" (public)?
>

That should be possible, good idea


> >> +
> >>  /**
> >>   * qemu_chr_fe_take_focus:
> >>   *
> >> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> >> index f3530a90e6..90cd7db007 100644
> >> --- a/chardev/char-fe.c
> >> +++ b/chardev/char-fe.c
> >> @@ -246,15 +246,67 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
> >>      }
> >>  }
> >>
> >> -void qemu_chr_fe_set_handlers_full(CharBackend *b,
> >> -                                   IOCanReadHandler *fd_can_read,
> >> -                                   IOReadHandler *fd_read,
> >> -                                   IOEventHandler *fd_event,
> >> -                                   BackendChangeHandler *be_change,
> >> -                                   void *opaque,
> >> -                                   GMainContext *context,
> >> -                                   bool set_open,
> >> -                                   bool sync_state)
> >> +struct MainContextWait {
> >> +    QemuCond cond;
> >> +    QemuMutex lock;
> >> +};
> >> +
> >> +static gboolean
> >> +main_context_wait_cb(gpointer user_data)
> >> +{
> >> +    struct MainContextWait *w = user_data;
> >> +
> >> +    qemu_mutex_lock(&w->lock);
> >> +    qemu_cond_signal(&w->cond);
> >> +    /* wait until switching is over */
> >> +    qemu_cond_wait(&w->cond, &w->lock);
> >
> > Could previous signal() directly wake up itself here?  Man
> > pthread_cond_broadcast says:
> >
> >        The pthread_cond_signal() function shall unblock at least one
> >        of the threads that are blocked on the specified condition
> >        variable cond (if any threads are blocked on cond).
> >
> >        If more than one thread is blocked on a condition variable, the
> >        scheduling policy shall determine the order in which threads
> >        are unblocked.
> >
> > So AFAIU it could, because neither there's a restriction on ordering
> > of how waiters are waked up, nor there's a limitation on how many
> > waiters will be waked up by a single signal().
> >
> > Why not simply use two semaphores?  Then locks can be avoided too.
> >
> > Regards,
> >
>
>