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 <marcandre.lureau@redhat.com>
---
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);
+ qemu_mutex_unlock(&w->lock);
+
+ qemu_mutex_destroy(&w->lock);
+ qemu_cond_destroy(&w->cond);
+ g_free(w);
+
+ return G_SOURCE_REMOVE;
+}
+
+static void
+main_context_wait(struct MainContextWait **wait, GMainContext *ctxt)
+{
+ struct MainContextWait *w = NULL;
+
+ if (!g_main_context_acquire(ctxt)) {
+ w = g_new0(struct MainContextWait, 1);
+ qemu_mutex_init(&w->lock);
+ qemu_cond_init(&w->cond);
+ qemu_mutex_lock(&w->lock);
+ g_main_context_invoke(ctxt, main_context_wait_cb, w);
+ /* wait for the context to freeze */
+ qemu_cond_wait(&w->cond, &w->lock);
+ qemu_mutex_unlock(&w->lock);
+ }
+
+ *wait = w;
+}
+
+static void
+main_context_resume(struct MainContextWait *wait, GMainContext *ctxt)
+{
+ if (wait) {
+ qemu_cond_signal(&wait->cond);
+ } else {
+ g_main_context_release(ctxt);
+ }
+}
+
+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 *new_context,
+ bool set_open,
+ bool sync_state)
{
Chardev *s;
int fe_open;
@@ -276,7 +328,7 @@ void qemu_chr_fe_set_handlers_full(CharBackend *b,
b->chr_be_change = be_change;
b->opaque = opaque;
- qemu_chr_be_update_read_handlers(s, context);
+ qemu_chr_be_update_read_handlers(s, new_context);
if (set_open) {
qemu_chr_fe_set_open(b, fe_open);
@@ -292,6 +344,34 @@ void qemu_chr_fe_set_handlers_full(CharBackend *b,
}
}
+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 *new_context,
+ bool set_open,
+ bool sync_state)
+{
+ GMainContext *old_context = b->chr->gcontext;
+ struct MainContextWait *old_ctxt_wait, *new_ctxt_wait;
+
+ main_context_wait(&old_ctxt_wait, old_context);
+ if (old_context != new_context) {
+ main_context_wait(&new_ctxt_wait, new_context);
+ }
+
+ qemu_chr_fe_set_handlers_internal(b, fd_can_read, fd_read, fd_event,
+ be_change, opaque, new_context,
+ set_open, sync_state);
+
+ main_context_resume(old_ctxt_wait, old_context);
+ if (old_context != new_context) {
+ main_context_resume(new_ctxt_wait, new_context);
+ }
+}
+
void qemu_chr_fe_set_handlers(CharBackend *b,
IOCanReadHandler *fd_can_read,
IOReadHandler *fd_read,
@@ -302,8 +382,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
bool set_open)
{
qemu_chr_fe_set_handlers_full(b, fd_can_read, fd_read, fd_event, be_change,
- opaque, context, set_open,
- true);
+ opaque, context, set_open, true);
}
void qemu_chr_fe_take_focus(CharBackend *b)
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 23aa82125d..9830cc4b37 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -283,13 +283,13 @@ static void mux_chr_update_read_handlers(Chardev *chr)
MuxChardev *d = MUX_CHARDEV(chr);
/* Fix up the real driver with mux routines */
- qemu_chr_fe_set_handlers_full(&d->chr,
- mux_chr_can_read,
- mux_chr_read,
- mux_chr_event,
- NULL,
- chr,
- chr->gcontext, true, false);
+ qemu_chr_fe_set_handlers_internal(&d->chr,
+ mux_chr_can_read,
+ mux_chr_read,
+ mux_chr_event,
+ NULL,
+ chr,
+ chr->gcontext, true, false);
}
void mux_set_focus(Chardev *chr, int focus)
--
2.21.0.rc1
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 <marcandre.lureau@redhat.com>
> ---
> 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
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 <marcandre.lureau@redhat.com>
>> ---
>> 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,
>
Hi
Le ven. 22 févr. 2019 à 09:33, Philippe Mathieu-Daudé <philmd@redhat.com> 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 <marcandre.lureau@redhat.com>
> >> ---
> >> 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,
> >
>
>
On Thu, Feb 21, 2019 at 04:03:57PM +0800, Peter Xu wrote:
[...]
> > +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.
Please feel free to skip this question. I think when cond_signal()
right before cond_wait() this thread is not yet in the waiting list so
at least my question seems invalid. Then cond+lock looks fine
comparing to sems. Sorry for the noise.
Regards,
--
Peter Xu
© 2016 - 2025 Red Hat, Inc.