Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap

[Marc-André Lureau]

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