* [PATCH] xen-bus: reduce scope of backend watch
@ 2020-09-23 15:57 Paul Durrant
2020-09-30 11:42 ` Anthony PERARD
0 siblings, 1 reply; 3+ messages in thread
From: Paul Durrant @ 2020-09-23 15:57 UTC (permalink / raw)
To: xen-devel, qemu-devel
Cc: Paul Durrant, Jerome Leseinne, Edwin Torok, Stefano Stabellini,
Anthony Perard
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>
---
Cc: Edwin Torok <edvin.torok@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
hw/xen/xen-backend.c | 10 ++++++++++
hw/xen/xen-bus.c | 38 ++++++++++++++++++++++++++++--------
include/hw/xen/xen-backend.h | 1 +
include/hw/xen/xen-bus.h | 3 ++-
4 files changed, 43 insertions(+), 9 deletions(-)
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;
};
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xen-bus: reduce scope of backend watch
2020-09-23 15:57 [PATCH] xen-bus: reduce scope of backend watch Paul Durrant
@ 2020-09-30 11:42 ` Anthony PERARD
2020-10-01 8:03 ` Paul Durrant
0 siblings, 1 reply; 3+ messages in thread
From: Anthony PERARD @ 2020-09-30 11:42 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, qemu-devel, Paul Durrant, Jerome Leseinne,
Edwin Torok, Stefano Stabellini
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,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] xen-bus: reduce scope of backend watch
2020-09-30 11:42 ` Anthony PERARD
@ 2020-10-01 8:03 ` Paul Durrant
0 siblings, 0 replies; 3+ messages in thread
From: Paul Durrant @ 2020-10-01 8:03 UTC (permalink / raw)
To: 'Anthony PERARD'
Cc: xen-devel, qemu-devel, 'Paul Durrant',
'Jerome Leseinne', 'Edwin Torok',
'Stefano Stabellini'
> -----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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-01 8:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-23 15:57 [PATCH] xen-bus: reduce scope of backend watch Paul Durrant
2020-09-30 11:42 ` Anthony PERARD
2020-10-01 8:03 ` Paul Durrant
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).