Hi
Le ven. 22 févr. 2019 à 09:33, Philippe Mathieu-Daudé <phi...@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.lur...@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,
> >
>
>
