[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap |
Date: |
Fri, 09 Jun 2017 14:53:35 +0000 |
On Thu, Jun 1, 2017 at 3:23 PM Anton Nefedov <address@hidden>
wrote:
>
>
> On 05/31/2017 10:20 PM, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, May 30, 2017 at 6:12 PM Anton Nefedov
> > <address@hidden <mailto:address@hidden>>
> wrote:
> >
> > This patch adds a possibility to change a char device without a
> frontend
> > removal.
> >
> > 1. Ideally, it would have to happen transparently to a frontend, i.e.
> > frontend would continue its regular operation.
> > However, backends are not stateless and are set up by the frontends
> > via qemu_chr_fe_<> functions, and it's not (generally) possible to
> > replay
> > that setup entirely in a backend code, as different chardevs respond
> > to the setup calls differently, so do frontends work differently
> basing
> > on those setup responses.
> > Moreover, some frontend can generally get and save the backend
> pointer
> > (qemu_chr_fe_get_driver()), and it will become invalid after backend
> > change.
> >
> > So, a frontend which would like to support chardev hotswap has to
> > register
> > a "backend change" handler, and redo its backend setup there.
> >
> > 2. Write path can be used by multiple threads and thus protected with
> > chr_write_lock.
> > So hotswap also has to be protected so write functions won't access
> > a backend being replaced.
> >
> >
> > Tbh, I don't understand the need for a different lock. Could you
> > explain? Even better would be to write a test that shows in which way
> > the lock helps.
> >
>
> hi Marc-André,
>
> The existing chr_write_lock belongs to Chardev.
> For the hotswap case, we need to ensure that be->chr won't change and
> the old Chardev (together with its mutex) won't be destroyed while it's
> used in the write functions.
>
> Maybe we could move the lock to CharBackend, instead of creating a new
> one. But I guess this
> 1. won't work for mux, where multiple CharBackends share the same
> Chardev
> 2. won't work for some chardevs, like pty which uses the lock for the
> timer handler
>
> Sorry if I'm not explaining clearly enough or maybe I'm missing some
> easier solution?
>
>
It looks to me like you would get the same guarantees by using the
chr_write_lock directly:
@@ -1381,7 +1374,7 @@ ChardevReturn *qmp_chardev_change(const char *id,
ChardevBackend *backend,
closed_sent = true;
}
- qemu_mutex_lock(&be->chr_lock);
+ qemu_mutex_lock(&chr->chr_write_lock);
chr->be = NULL;
qemu_chr_fe_connect(be, chr_new, &error_abort);
@@ -1389,14 +1382,14 @@ ChardevReturn *qmp_chardev_change(const char *id,
ChardevBackend *backend,
error_setg(errp, "Chardev '%s' change failed", chr_new->label);
chr_new->be = NULL;
qemu_chr_fe_connect(be, chr, &error_abort);
- qemu_mutex_unlock(&be->chr_lock);
+ qemu_mutex_unlock(&chr->chr_write_lock);
if (closed_sent) {
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
}
object_unref(OBJECT(chr_new));
return NULL;
}
- qemu_mutex_unlock(&be->chr_lock);
+ qemu_mutex_unlock(&chr->chr_write_lock);
I wonder if we should rename 'chr_write_lock' to just 'lock' :)
> I can try to add a test but can't quite see yet how to freeze the old
> chardev somewhere in cc->chr_write() and hotswap it while it's there.
>
>
> >
> >
> > Signed-off-by: Anton Nefedov <address@hidden
> > <mailto:address@hidden>>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden
> > <mailto:address@hidden>>
> >
> >
> > patch looks good otherwise
> >
> > ---
> > chardev/char.c | 126
> > ++++++++++++++++++++++++++++++++++++++++++++++----
> > include/sysemu/char.h | 9 ++++
> > qapi-schema.json | 40 ++++++++++++++++
> > 3 files changed, 165 insertions(+), 10 deletions(-)
> >
> > --
> > Marc-André Lureau
>
> /Anton
>
--
Marc-André Lureau