[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] chardev: make qemu_chr_fe_set_handlers() co
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] chardev: make qemu_chr_fe_set_handlers() context switching safer |
Date: |
Fri, 22 Feb 2019 09:32:59 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 |
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,
>
[Qemu-devel] [PATCH 3/4] monitor: set the chardev context from the main context/thread, Marc-André Lureau, 2019/02/20
[Qemu-devel] [PATCH 4/4] char-socket: restart the reconnect timer to switch context, Marc-André Lureau, 2019/02/20
Re: [Qemu-devel] [PATCH 0/4] RFC: make chardev context switching safer, Peter Xu, 2019/02/21