Re: [Qemu-devel] [for-2.11 PATCH 13/26] qdev: store DeviceState's canonical path to use when unparenting

[David Gibson]

On Thu, Jul 27, 2017 at 06:50:42PM +0200, Greg Kurz wrote:
> On Wed, 26 Jul 2017 15:24:43 +1000
> David Gibson <address@hidden> wrote:
> 
> > On Tue, Jul 25, 2017 at 08:00:47PM +0200, Greg Kurz wrote:
> > > From: Michael Roth <address@hidden>
> > > 
> > > device_unparent(dev, ...) is called when a device is unparented,
> > > either directly, or as a result of a parent device being
> > > finalized, and handles some final cleanup for the device. Part
> > > of this includes emiting a DEVICE_DELETED QMP event to notify
> > > management, which includes the device's path in the composition
> > > tree as provided by object_get_canonical_path().
> > > 
> > > object_get_canonical_path() assumes the device is still connected
> > > to the machine/root container, and will assert otherwise, but
> > > in some situations this isn't the case:
> > > 
> > > If the parent is finalized as a result of object_unparent(), it
> > > will still be attached to the composition tree at the time any
> > > children are unparented as a result of that same call to
> > > object_unparent(). However, in some cases, object_unparent()
> > > will complete without finalizing the parent device, due to
> > > lingering references that won't be released till some time later.
> > > One such example is if the parent has MemoryRegion children (which
> > > take a ref on their parent), who in turn have AddressSpace's (which
> > > take a ref on their regions), since those AddressSpaces get cleaned
> > > up asynchronously by the RCU thread.
> > > 
> > > In this case qdev:device_unparent() may be called for a child Device
> > > that no longer has a path to the root/machine container, causing
> > > object_get_canonical_path() to assert.
> > > 
> > > Fix this by storing the canonical path during realize() so the
> > > information will still be available for device_unparent() in such
> > > cases.  
> > 
> > Hm.  I'm no expert on the QOM model, but I'm not sure this is the
> > right approach.
> > 
> > I would have thought the right time to emit the DEVICE_DELETED message
> > would be when the device leaves the main composition tree, even if it
> > could be finalized later.
> > 
> > If we made that the case, does this problem go away?
> > 
> 
> I'm no expert either and I confess I took this patch simply because it was
> in Michael's original patchset. :)
> 
> But according to Michael's answer, it seems that the issue has a broader
> scope than just PHB hotplug...

Ok.  I see Michael has posted this and a couple of other things
separately.  Let's hope that can get resolved upstream, and rebase
this series on top of the result.

> 
> > > Cc: Michael S. Tsirkin <address@hidden>
> > > Cc: Paolo Bonzini <address@hidden>
> > > Signed-off-by: Michael Roth <address@hidden>
> > > Signed-off-by: Greg Kurz <address@hidden>
> > > ---
> > > Changes since RFC:
> > > - rebased against ppc-for-2.10
> > > ---
> > >  hw/core/qdev.c         |   15 ++++++++++++---
> > >  include/hw/qdev-core.h |    1 +
> > >  2 files changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > index 606ab53c42cd..a64b35c16251 100644
> > > --- a/hw/core/qdev.c
> > > +++ b/hw/core/qdev.c
> > > @@ -928,6 +928,12 @@ static void device_set_realized(Object *obj, bool 
> > > value, Error **errp)
> > >              goto post_realize_fail;
> > >          }
> > >  
> > > +        /* always re-initialize since we clean up in device_unparent() 
> > > instead
> > > +         * of unrealize()
> > > +         */
> > > +        g_free(dev->canonical_path);
> > > +        dev->canonical_path = object_get_canonical_path(OBJECT(dev));
> > > +
> > >          if (qdev_get_vmsd(dev)) {
> > >              if (vmstate_register_with_alias_id(dev, -1, 
> > > qdev_get_vmsd(dev), dev,
> > >                                                 dev->instance_id_alias,
> > > @@ -984,6 +990,7 @@ child_realize_fail:
> > >      }
> > >  
> > >  post_realize_fail:
> > > +    g_free(dev->canonical_path);
> > >      if (dc->unrealize) {
> > >          dc->unrealize(dev, NULL);
> > >      }
> > > @@ -1102,10 +1109,12 @@ static void device_unparent(Object *obj)
> > >  
> > >      /* Only send event if the device had been completely realized */
> > >      if (dev->pending_deleted_event) {
> > > -        gchar *path = object_get_canonical_path(OBJECT(dev));
> > > +        g_assert(dev->canonical_path);
> > >  
> > > -        qapi_event_send_device_deleted(!!dev->id, dev->id, path, 
> > > &error_abort);
> > > -        g_free(path);
> > > +        qapi_event_send_device_deleted(!!dev->id, dev->id, 
> > > dev->canonical_path,
> > > +                                       &error_abort);
> > > +        g_free(dev->canonical_path);
> > > +        dev->canonical_path = NULL;
> > >      }
> > >  
> > >      qemu_opts_del(dev->opts);
> > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > > index ae317286a480..9237b6849ff3 100644
> > > --- a/include/hw/qdev-core.h
> > > +++ b/include/hw/qdev-core.h
> > > @@ -153,6 +153,7 @@ struct DeviceState {
> > >      /*< public >*/
> > >  
> > >      const char *id;
> > > +    char *canonical_path;
> > >      bool realized;
> > >      bool pending_deleted_event;
> > >      QemuOpts *opts;
> > >   
> > 
> 



-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature