On Thu, Jun 1, 2017 at 3:23 PM Anton Nefedov <anton.nefe...@virtuozzo.com> wrote:
> > > On 05/31/2017 10:20 PM, Marc-André Lureau wrote: > > Hi > > > > On Tue, May 30, 2017 at 6:12 PM Anton Nefedov > > <anton.nefe...@virtuozzo.com <mailto:anton.nefe...@virtuozzo.com>> > 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 <anton.nefe...@virtuozzo.com > > <mailto:anton.nefe...@virtuozzo.com>> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com > > <mailto:vsement...@virtuozzo.com>> > > > > > > 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