Re: [Qemu-devel] [RFCv2 1/2] spapr: Remove unnecessary owner field from sPAPRDRConnector

[David Gibson]

On Mon, Sep 14, 2015 at 04:24:23PM +0200, Paolo Bonzini wrote:
> 
> 
> On 14/09/2015 16:06, Alexey Kardashevskiy wrote:
> >>>>>
> >>>>> === * There is no way for a child to determine what its parent
> >>>>> is.  It is not * a bidirectional relationship.  This is by
> >>>>> design. ===
> >>>>>
> >>>>> This part always confused me as there is "Object *parent" in
> >>>>> the "struct Object". So there is way to determine but it must
> >>>>> not be used? Is it debug only?
> >>>>>
> >>>>> Anyway, all members of the Object class are under /*< private
> >>>>>> */ so they should not be accesses in sPAPR code, I believe.
> >>> Ah, good point, I missed that.  I guess we have to keep the owner
> >>> field, redundant though it seems.  Blech.
> >>
> >> I think the comment is wrong or at least inaccurate; it only applies
> >> to the external QOM interface.
> > 
> > Is this case external?
> 
> I meant external as in qom-get, qom-set, qom-list.  There isn't a ".."
> property.
> 
> > Originally I was looking for a object_get_parent() but it is not there
> > so I decided that the comment is correct or I just fail to understand it :)
> 
> Yes, we can add such an API.
> 
> Let's look also at what ->owner is used for.
> 
> > object_property_add_alias(root_container, link_name,
> >                           drc->owner, child_name, &err);
> 
> This can be rewritten as
> 
>      object_property_add_const_link(root_container, link_name,
>                                     drc, &err);
> 
> >     QTAILQ_FOREACH(prop, &root_container->properties, node) {
> >         Object *obj;
> >         sPAPRDRConnector *drc;
> >         sPAPRDRConnectorClass *drck;
> >         uint32_t drc_index, drc_power_domain;
> > 
> >         if (!strstart(prop->type, "link<", NULL)) {
> >             continue;
> >         }
> > 
> >         obj = object_property_get_link(root_container, prop->name, NULL);
> >         drc = SPAPR_DR_CONNECTOR(obj);
> >         drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > 
> >         if (owner && (drc->owner != owner)) {
> 
> Could the PCI host bridge instead store the DR connectors when it
> creates them with spapr_dr_connector_new?  Then you can just call
> spapr_drc_populate_dt directly with the right objects, and avoid another
> O(n^2) loop.

So, yes, iterating over the connectors under the owner rather than
going via the global links is another cleanup I've considered but
haven't gotten around to.

Except, I've been thinking further, and I'm not sure it makes sense to
keep these DR connectors around as full QOM objects anyway.  Having
them here at least potentially exposes the DR connector stuff to
users, when it's really an internal of how the hypervisor communicates
with the guest about hotplug.

I'm going to have to talk to more QOM-experienced people to thrash out
the details, but I'm thinking a better approach would be to add a "DR
connector array" QOM interface to the existing PCI host bridge objects
and.. to something for CPU and memory.  That interface would allow the
necessary lookups specifically for DR hotplug events and avoid
creating thousands of extra QOM objects.

Either way, it makes this little cleanup pretty irrelevant, so I'll
drop it for now.

-- 
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

pgpBqSprfICna.pgp
Description: PGP signature