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

[Anton Nefedov]

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?

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