From: David Gibson <david@gibson.dropbear.id.au>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: mdroth@linux.vnet.ibm.com, Alexey Kardashevskiy <aik@ozlabs.ru>,
agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org,
bharata@linux.vnet.ibm.com
Subject: Re: [Qemu-devel] [RFCv2 1/2] spapr: Remove unnecessary owner field from sPAPRDRConnector
Date: Wed, 16 Sep 2015 13:16:35 +1000 [thread overview]
Message-ID: <20150916031635.GY2547@voom.fritz.box> (raw)
In-Reply-To: <55F6D897.1090100@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3487 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
next prev parent reply other threads:[~2015-09-16 3:25 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-14 1:41 [Qemu-devel] [RFCv2 0/2] spapr: Cleanups to dynamic reconfiguration mechanism David Gibson
2015-09-14 1:41 ` [Qemu-devel] [RFCv2 1/2] spapr: Remove unnecessary owner field from sPAPRDRConnector David Gibson
2015-09-14 6:23 ` Bharata B Rao
2015-09-14 8:22 ` Alexey Kardashevskiy
2015-09-14 11:45 ` David Gibson
2015-09-14 12:11 ` Paolo Bonzini
2015-09-14 14:06 ` Alexey Kardashevskiy
2015-09-14 14:24 ` Paolo Bonzini
2015-09-16 3:16 ` David Gibson [this message]
2015-09-17 15:50 ` Michael Roth
2015-09-17 15:53 ` Paolo Bonzini
2015-09-17 23:03 ` Michael Roth
2015-09-17 15:01 ` Michael Roth
2015-09-15 0:31 ` David Gibson
2015-09-15 0:30 ` David Gibson
2015-09-15 11:05 ` Paolo Bonzini
2015-09-14 1:41 ` [Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors David Gibson
2015-09-14 4:07 ` Bharata B Rao
2015-09-14 4:14 ` David Gibson
2015-09-14 4:41 ` Bharata B Rao
2015-09-14 5:20 ` David Gibson
2015-09-14 8:13 ` Alexey Kardashevskiy
2015-09-15 4:03 ` Michael Roth
2015-09-15 4:21 ` Michael Roth
2015-09-16 3:18 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150916031635.GY2547@voom.fritz.box \
--to=david@gibson.dropbear.id.au \
--cc=agraf@suse.de \
--cc=aik@ozlabs.ru \
--cc=bharata@linux.vnet.ibm.com \
--cc=mdroth@linux.vnet.ibm.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.