Re: [PATCH] xen-bus: reduce scope of backend watch

[Anthony PERARD <anthony.perard@citrix.com>]

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