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

[Peter Xu]

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);
> +
>  /**
>   * 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,

-- 
Peter Xu