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

[Philippe Mathieu-Daudé]

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)?

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