On Thu, Jun 1, 2017 at 3:23 PM Anton Nefedov
<address@hidden <mailto: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>
<mailto: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' :)