| Message ID | 20200923155731.29528-1-paul@xen.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen-bus: reduce scope of backend watch | expand |
On Wed, Sep 23, 2020 at 04:57:31PM +0100, Paul Durrant wrote: > From: Paul Durrant <pdurrant@amazon.com> > > Currently a single watch on /local/domain/X/backend is registered by each > QEMU process running in service domain X (where X is usually 0). The purpose > of this watch is to ensure that QEMU is notified when the Xen toolstack > creates a new device backend area. > Such a backend area is specific to a single frontend area created for a > specific guest domain and, since each QEMU process is also created to service > a specfic guest domain, it is unnecessary and inefficient to notify all QEMU > processes. > Only the QEMU process associated with the same guest domain need > receive the notification. This patch re-factors the watch registration code > such that notifications are targetted appropriately. > > Reported-by: Jerome Leseinne <jerome.leseinne@gmail.com> > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c > index 10199fb58d..f2711fe4a7 100644 > --- a/hw/xen/xen-backend.c > +++ b/hw/xen/xen-backend.c > @@ -41,6 +41,11 @@ static void xen_backend_table_add(XenBackendImpl *impl) > g_hash_table_insert(xen_backend_table_get(), (void *)impl->type, impl); > } > > +static void **xen_backend_table_keys(unsigned int *count) > +{ > + return g_hash_table_get_keys_as_array(xen_backend_table_get(), count); That could be cast to (const gchar **) as the GLib doc suggest, or (const char **) since gchar and char are the same. https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-get-keys-as-array > +} > + > static const XenBackendImpl *xen_backend_table_lookup(const char *type) > { > return g_hash_table_lookup(xen_backend_table_get(), type); > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > index 9ce1c9540b..c83da93bf3 100644 > --- a/hw/xen/xen-bus.c > +++ b/hw/xen/xen-bus.c > @@ -430,7 +430,13 @@ static void xen_bus_unrealize(BusState *bus) > trace_xen_bus_unrealize(); > > if (xenbus->backend_watch) { > - xen_bus_remove_watch(xenbus, xenbus->backend_watch, NULL); > + unsigned int i; > + > + for (i = 0; i < xenbus->backend_types; i++) { > + xen_bus_remove_watch(xenbus, xenbus->backend_watch[i], NULL); We should check if backend_watch[i] is NULL. > + } > + > + g_free(xenbus->backend_watch); > xenbus->backend_watch = NULL; > } > The rest of the patch looks fine. Next improvement is to only look at only one backend type in xen_bus_backend_changed() since there is now a watch per backend type :-), but that would be for another day. Cheers,
> -----Original Message----- > From: Anthony PERARD <anthony.perard@citrix.com> > Sent: 30 September 2020 12:43 > To: Paul Durrant <paul@xen.org> > Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; Paul Durrant <pdurrant@amazon.com>; Jerome > Leseinne <jerome.leseinne@gmail.com>; Edwin Torok <edvin.torok@citrix.com>; Stefano Stabellini > <sstabellini@kernel.org> > Subject: Re: [PATCH] xen-bus: reduce scope of backend watch > > On Wed, Sep 23, 2020 at 04:57:31PM +0100, Paul Durrant wrote: > > From: Paul Durrant <pdurrant@amazon.com> > > > > Currently a single watch on /local/domain/X/backend is registered by each > > QEMU process running in service domain X (where X is usually 0). The purpose > > of this watch is to ensure that QEMU is notified when the Xen toolstack > > creates a new device backend area. > > Such a backend area is specific to a single frontend area created for a > > specific guest domain and, since each QEMU process is also created to service > > a specfic guest domain, it is unnecessary and inefficient to notify all QEMU > > processes. > > Only the QEMU process associated with the same guest domain need > > receive the notification. This patch re-factors the watch registration code > > such that notifications are targetted appropriately. > > > > Reported-by: Jerome Leseinne <jerome.leseinne@gmail.com> > > Signed-off-by: Paul Durrant <pdurrant@amazon.com> > > > > diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c > > index 10199fb58d..f2711fe4a7 100644 > > --- a/hw/xen/xen-backend.c > > +++ b/hw/xen/xen-backend.c > > @@ -41,6 +41,11 @@ static void xen_backend_table_add(XenBackendImpl *impl) > > g_hash_table_insert(xen_backend_table_get(), (void *)impl->type, impl); > > } > > > > +static void **xen_backend_table_keys(unsigned int *count) > > +{ > > + return g_hash_table_get_keys_as_array(xen_backend_table_get(), count); > > That could be cast to (const gchar **) as the GLib doc suggest, or (const > char **) since gchar and char are the same. > https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-get-keys-as-array > Ok, I'll re-arrange the const-ing to cast at the inner level. > > +} > > + > > static const XenBackendImpl *xen_backend_table_lookup(const char *type) > > { > > return g_hash_table_lookup(xen_backend_table_get(), type); > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c > > index 9ce1c9540b..c83da93bf3 100644 > > --- a/hw/xen/xen-bus.c > > +++ b/hw/xen/xen-bus.c > > @@ -430,7 +430,13 @@ static void xen_bus_unrealize(BusState *bus) > > trace_xen_bus_unrealize(); > > > > if (xenbus->backend_watch) { > > - xen_bus_remove_watch(xenbus, xenbus->backend_watch, NULL); > > + unsigned int i; > > + > > + for (i = 0; i < xenbus->backend_types; i++) { > > + xen_bus_remove_watch(xenbus, xenbus->backend_watch[i], NULL); > > We should check if backend_watch[i] is NULL. > Yes, I'll add a check. > > + } > > + > > + g_free(xenbus->backend_watch); > > xenbus->backend_watch = NULL; > > } > > > > The rest of the patch looks fine. Next improvement is to only look at > only one backend type in xen_bus_backend_changed() since there is now a > watch per backend type :-), but that would be for another day. > Might not be too tricky. I'll see if I can come up with a follow-up patch. Paul > Cheers, > > -- > Anthony PERARD
diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c index 10199fb58d..f2711fe4a7 100644 --- a/hw/xen/xen-backend.c +++ b/hw/xen/xen-backend.c @@ -41,6 +41,11 @@ static void xen_backend_table_add(XenBackendImpl *impl) g_hash_table_insert(xen_backend_table_get(), (void *)impl->type, impl); } +static void **xen_backend_table_keys(unsigned int *count) +{ + return g_hash_table_get_keys_as_array(xen_backend_table_get(), count); +} + static const XenBackendImpl *xen_backend_table_lookup(const char *type) { return g_hash_table_lookup(xen_backend_table_get(), type); @@ -70,6 +75,11 @@ void xen_backend_register(const XenBackendInfo *info) xen_backend_table_add(impl); } +const char **xen_backend_get_types(unsigned int *count) +{ + return (const char **)xen_backend_table_keys(count); +} + static QLIST_HEAD(, XenBackendInstance) backend_list; static void xen_backend_list_add(XenBackendInstance *backend) diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c index 9ce1c9540b..c83da93bf3 100644 --- a/hw/xen/xen-bus.c +++ b/hw/xen/xen-bus.c @@ -430,7 +430,13 @@ static void xen_bus_unrealize(BusState *bus) trace_xen_bus_unrealize(); if (xenbus->backend_watch) { - xen_bus_remove_watch(xenbus, xenbus->backend_watch, NULL); + unsigned int i; + + for (i = 0; i < xenbus->backend_types; i++) { + xen_bus_remove_watch(xenbus, xenbus->backend_watch[i], NULL); + } + + g_free(xenbus->backend_watch); xenbus->backend_watch = NULL; } @@ -446,8 +452,11 @@ static void xen_bus_unrealize(BusState *bus) static void xen_bus_realize(BusState *bus, Error **errp) { + char *key = g_strdup_printf("%u", xen_domid); XenBus *xenbus = XEN_BUS(bus); unsigned int domid; + const char **type; + unsigned int i; Error *local_err = NULL; trace_xen_bus_realize(); @@ -469,19 +478,32 @@ static void xen_bus_realize(BusState *bus, Error **errp) module_call_init(MODULE_INIT_XEN_BACKEND); - xenbus->backend_watch = - xen_bus_add_watch(xenbus, "", /* domain root node */ - "backend", xen_bus_backend_changed, &local_err); - if (local_err) { - /* This need not be treated as a hard error so don't propagate */ - error_reportf_err(local_err, - "failed to set up enumeration watch: "); + type = xen_backend_get_types(&xenbus->backend_types); + xenbus->backend_watch = g_new(XenWatch *, xenbus->backend_types); + + for (i = 0; i < xenbus->backend_types; i++) { + char *node = g_strdup_printf("backend/%s", type[i]); + + xenbus->backend_watch[i] = + xen_bus_add_watch(xenbus, node, key, xen_bus_backend_changed, + &local_err); + if (local_err) { + /* This need not be treated as a hard error so don't propagate */ + error_reportf_err(local_err, + "failed to set up '%s' enumeration watch: ", + type[i]); + } + + g_free(node); } + g_free(type); + g_free(key); return; fail: xen_bus_unrealize(bus); + g_free(key); } static void xen_bus_unplug_request(HotplugHandler *hotplug, diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h index 010d712638..aac2fd454d 100644 --- a/include/hw/xen/xen-backend.h +++ b/include/hw/xen/xen-backend.h @@ -31,6 +31,7 @@ void xen_backend_set_device(XenBackendInstance *backend, XenDevice *xen_backend_get_device(XenBackendInstance *backend); void xen_backend_register(const XenBackendInfo *info); +const char **xen_backend_get_types(unsigned int *nr); void xen_backend_device_create(XenBus *xenbus, const char *type, const char *name, QDict *opts, Error **errp); diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h index 3df696136f..6bdbf3ff82 100644 --- a/include/hw/xen/xen-bus.h +++ b/include/hw/xen/xen-bus.h @@ -66,7 +66,8 @@ struct XenBus { domid_t backend_id; struct xs_handle *xsh; XenWatchList *watch_list; - XenWatch *backend_watch; + unsigned int backend_types; + XenWatch **backend_watch; QLIST_HEAD(, XenDevice) inactive_devices; };