[Qemu-devel] [RFCv2 0/2] spapr: Cleanups to dynamic reconfiguration mechanism

[Qemu-devel] [RFCv2 0/2] spapr: Cleanups to dynamic reconfiguration mechanism

[David Gibson]

Here are some cleanups and improvements to the "dynamic
reconfiguration" (hotplug) infrastructure for the "pseries" machine
type.

There's an improved version of my patch to mitigate the O(n^3) time
for large maxmem values, and another small cleanup to remove a
redundant field in the structure.

David Gibson (2):
  spapr: Remove unnecessary owner field from sPAPRDRConnector
  spapr: Don't use QOM [*] syntax for DR connectors.

 hw/ppc/spapr_drc.c         | 10 ++++++----
 include/hw/ppc/spapr_drc.h |  1 -
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.4.3

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

[David Gibson]

The sPAPRDRConnector pseudo-device contains an owner field which is
set in spapr_dr_connector_new().  However, that function also calls
object_property_add_child() to set the DRConnector as the QOM child of
the owner object.  That means that owner is always the same as the QOM
parent, and so redundant.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c         | 5 ++---
 include/hw/ppc/spapr_drc.h | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 9ce844a..68e0c3e 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -416,7 +416,7 @@ static void realize(DeviceState *d, Error **errp)
     child_name = object_get_canonical_path_component(OBJECT(drc));
     DPRINTFN("drc child name: %s", child_name);
     object_property_add_alias(root_container, link_name,
-                              drc->owner, child_name, &err);
+                              OBJECT(drc)->parent, child_name, &err);
     if (err) {
         error_report("%s", error_get_pretty(err));
         error_free(err);
@@ -456,7 +456,6 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
 
     drc->type = type;
     drc->id = id;
-    drc->owner = owner;
     object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
     object_property_set_bool(OBJECT(drc), true, "realized", NULL);
 
@@ -669,7 +668,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
         drc = SPAPR_DR_CONNECTOR(obj);
         drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
-        if (owner && (drc->owner != owner)) {
+        if (owner && (OBJECT(drc)->parent != owner)) {
             continue;
         }
 
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 28ffeae..16e2d4b 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -137,7 +137,6 @@ typedef struct sPAPRDRConnector {
 
     sPAPRDRConnectorType type;
     uint32_t id;
-    Object *owner;
     const char *name;
 
     /* sensor/indicator states */
-- 
2.4.3

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

[Bharata B Rao]

On Mon, Sep 14, 2015 at 11:41:52AM +1000, David Gibson wrote:
> The sPAPRDRConnector pseudo-device contains an owner field which is
> set in spapr_dr_connector_new().  However, that function also calls
> object_property_add_child() to set the DRConnector as the QOM child of
> the owner object.  That means that owner is always the same as the QOM
> parent, and so redundant.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Tested CPU and memory hotplug with reboot and migration.

Tested-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

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

[Alexey Kardashevskiy]

On 09/14/2015 11:41 AM, David Gibson wrote:
> The sPAPRDRConnector pseudo-device contains an owner field which is
> set in spapr_dr_connector_new().  However, that function also calls
> object_property_add_child() to set the DRConnector as the QOM child of
> the owner object.  That means that owner is always the same as the QOM
> parent, and so redundant.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   hw/ppc/spapr_drc.c         | 5 ++---
>   include/hw/ppc/spapr_drc.h | 1 -
>   2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 9ce844a..68e0c3e 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -416,7 +416,7 @@ static void realize(DeviceState *d, Error **errp)
>       child_name = object_get_canonical_path_component(OBJECT(drc));
>       DPRINTFN("drc child name: %s", child_name);
>       object_property_add_alias(root_container, link_name,
> -                              drc->owner, child_name, &err);
> +                              OBJECT(drc)->parent, child_name, &err);
>       if (err) {
>           error_report("%s", error_get_pretty(err));
>           error_free(err);
> @@ -456,7 +456,6 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>
>       drc->type = type;
>       drc->id = id;
> -    drc->owner = owner;
>       object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);


The comment for object_property_add_child() in include/qom/object.h says:

===
* 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.



>       object_property_set_bool(OBJECT(drc), true, "realized", NULL);
>
> @@ -669,7 +668,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>           drc = SPAPR_DR_CONNECTOR(obj);
>           drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>
> -        if (owner && (drc->owner != owner)) {
> +        if (owner && (OBJECT(drc)->parent != owner)) {
>               continue;
>           }
>
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 28ffeae..16e2d4b 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -137,7 +137,6 @@ typedef struct sPAPRDRConnector {
>
>       sPAPRDRConnectorType type;
>       uint32_t id;
> -    Object *owner;
>       const char *name;
>
>       /* sensor/indicator states */
>


-- 
Alexey

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2413 bytes --]

On Mon, Sep 14, 2015 at 06:22:35PM +1000, Alexey Kardashevskiy wrote:
> On 09/14/2015 11:41 AM, David Gibson wrote:
> >The sPAPRDRConnector pseudo-device contains an owner field which is
> >set in spapr_dr_connector_new().  However, that function also calls
> >object_property_add_child() to set the DRConnector as the QOM child of
> >the owner object.  That means that owner is always the same as the QOM
> >parent, and so redundant.
> >
> >Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >---
> >  hw/ppc/spapr_drc.c         | 5 ++---
> >  include/hw/ppc/spapr_drc.h | 1 -
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> >
> >diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> >index 9ce844a..68e0c3e 100644
> >--- a/hw/ppc/spapr_drc.c
> >+++ b/hw/ppc/spapr_drc.c
> >@@ -416,7 +416,7 @@ static void realize(DeviceState *d, Error **errp)
> >      child_name = object_get_canonical_path_component(OBJECT(drc));
> >      DPRINTFN("drc child name: %s", child_name);
> >      object_property_add_alias(root_container, link_name,
> >-                              drc->owner, child_name, &err);
> >+                              OBJECT(drc)->parent, child_name, &err);
> >      if (err) {
> >          error_report("%s", error_get_pretty(err));
> >          error_free(err);
> >@@ -456,7 +456,6 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> >
> >      drc->type = type;
> >      drc->id = id;
> >-    drc->owner = owner;
> >      object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
> 
> 
> The comment for object_property_add_child() in include/qom/object.h says:
> 
> ===
> * 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.

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

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

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 14/09/2015 13:45, David Gibson 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.  The patch is a good idea, even though
OBJECT(x)->y traditionally is not used (instead you assign OBJECT(x) to
a different Object* variable).

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJV9rmEAAoJEL/70l94x66DRYEH/3vWHTrKc5m3aoPgyxlSRCJf
dKWtYVhCJ4SFhhtW5v/XTB5ze6PWa8gg/Vv5hBUAhFAaoGkCx2jtLfP/+KS/oYT3
8x+JS9SpWQGjlsaIHGD91HenaOARafgsnJVNp4HqQPPz3IAfYP7noDNrVXhN4eQF
zjWnIVdfqD7nUW/f8zzVM3Xv7WEiV19K6foOUs/LTC6OEiqkOIAruvTqIAXn0MHe
bTeoWivfibO2Iomq6zExuNnSDjmqrdXuY4jmgMLzpzwrisczy2cCG0wbb0yzuK87
9UXtF90MNC/dIFMVrwmujruR2fWEvorIs+ZR+Udq2n5saau+4WUaX7ATOZzyVao=
=5cbm
-----END PGP SIGNATURE-----

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

[Alexey Kardashevskiy]

On 09/14/2015 10:11 PM, Paolo Bonzini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
>
>
> On 14/09/2015 13:45, David Gibson 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?

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


> The patch is a good idea, even though
> OBJECT(x)->y traditionally is not used (instead you assign OBJECT(x) to
> a different Object* variable).



-- 
Alexey

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

[Paolo Bonzini]

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.

Paolo

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

[David Gibson]

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

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

[Michael Roth]

Quoting David Gibson (2015-09-15 22:16:35)
> 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.

We still need globals for RTAS lookups. I think QOM is our most
mature/well-tested interface for managing inter-device
relationships/lookups, but I can understand if using
root_container/link<DRC> seems off. To me it seem like a nice
"freebie" we get from using QOM, and gave us nice guarantees like
globally unique paths to correspond to globally unique DRC indexes.

But it's a simple enough task that it can live elsewhere. It will
still be global though (RTAS + device_add + FDT generation all
needs to reference it), and I don't think adding more global
interfaces to do things already provided by QOM is worth it.

> 
> 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 think QOM should manage a machine's composition tree. The fact that
there's a side-effect of exposing things to users shouldn't prevent
us from making use of it. We expose all sorts of state about gpio
pins, memory regions, etc. currently. If there's a need to hide things
from external users, or better manage things like read/write permissions,
I think that logic should be added to QOM rather than used as an argument
against it.

Whether the current composition tree makes sense is tricky to answer.
pseries is guest-only, so everything is sort of a hypervisor internal,
but if a theoretical baremetal pseries existed, I think you could easily
end up with, in the case of PCI anyway, an SHPC-like device that's wired
up to a bus of some sort, addressed by DRC index, with registers to set
indicator/isolation states and internal logic to control state
transitions like SHPC has.

SHPCs are exposed to guests via PCI bus, but pseries calls for RTAS,
and SHPCs are modeled as an internal of the bus controller, whereas
things like memory/cpu don't really have a bus we can piggy-back on,
so we have something a little ad-hoc where DRCs are modeled as
self-contained devices.

FWIW, the initial implementation of hotplug did not use QOM, and was
rewritten to use it at the suggestion of Alex. I think the pieces
fall into place much more logically taking the latter approach and
modeling DRCs as devices. The alternative is to make DRC an internal
detail of PCI buses, and non-existant memory/cpu buses, which I think
will ultimately result is self-contained 'devices' that simple aren't
present in the composition tree. As a result I think things like
managing DRC creation/teardown (think PHB hotplug of hotplug capable
PCI buses) will become more tedious/ad-hoc since we lose the
parent/child life-cycle management that QOM gives us for free.

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

qdev can create up to 256*32 under /machine/unattached for PCI machines,
and for pseries as many as 256*32*32. It would've likely even done so
using the "[*]" interface if that was around at the time (but it does use
it for gpio's). I don't think the number of objects should limit what we
use QOM for (and don't think it does really, it's just the "[*]"
implementation that seems to be problematic)

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

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

[Paolo Bonzini]

On 17/09/2015 17:50, Michael Roth wrote:
> We still need globals for RTAS lookups. I think QOM is our most
> mature/well-tested interface for managing inter-device
> relationships/lookups, but I can understand if using
> root_container/link<DRC> seems off. To me it seem like a nice
> "freebie" we get from using QOM, and gave us nice guarantees like
> globally unique paths to correspond to globally unique DRC indexes.

I think it's okay; but I don't really like looking at ->properties
directly, without an API (which doesn't exist indeed).

Paolo

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

[Michael Roth]

Quoting Paolo Bonzini (2015-09-17 10:53:40)
> 
> 
> On 17/09/2015 17:50, Michael Roth wrote:
> > We still need globals for RTAS lookups. I think QOM is our most
> > mature/well-tested interface for managing inter-device
> > relationships/lookups, but I can understand if using
> > root_container/link<DRC> seems off. To me it seem like a nice
> > "freebie" we get from using QOM, and gave us nice guarantees like
> > globally unique paths to correspond to globally unique DRC indexes.
> 
> I think it's okay; but I don't really like looking at ->properties
> directly, without an API (which doesn't exist indeed).

Yah, rather than copying qmp_qom_list() I think a proper interface
would've been warranted.

Perhaps an object_link_foreach() that mirrors object_child_foreach()?
For maybe just a more generic object_foreach() with a type mask?

Another approach would be to re-use or genericize qmp_qom_list() but
that seems like extra allocations/cleanup that aren't really
necessary outside of QMP/QAPI return values.

> 
> Paolo
> 

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

[Michael Roth]

Quoting Paolo Bonzini (2015-09-14 09:24:23)
> 
> 
> 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.

It could be done I think, but in some cases the DRC lists can include a mix
of DRC types/indexes from multiple parents, so we'd need to add some logic
to modify/append to existing paths in FDT.

We end up needing a global registry of some sort anyway, since RTAS
calls from the guest address the DRCs purely by the DRC index, so I
think if we can make use of that same registry to simply the above case
there's no reason not to.

> 
> Paolo
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]

On Tue, Sep 15, 2015 at 12:06:49AM +1000, Alexey Kardashevskiy wrote:
> On 09/14/2015 10:11 PM, Paolo Bonzini wrote:
> >-----BEGIN PGP SIGNED MESSAGE-----
> >Hash: SHA256
> >
> >
> >
> >On 14/09/2015 13:45, David Gibson 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'm assyming by "external" Paolo means outside qemu - i.e. access via
the qapi monitor.

> 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 :)
> 
> 
> >The patch is a good idea, even though
> >OBJECT(x)->y traditionally is not used (instead you assign OBJECT(x) to
> >a different Object* variable).
> 
> 
> 

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

On Mon, Sep 14, 2015 at 02:11:53PM +0200, Paolo Bonzini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> 
> 
> On 14/09/2015 13:45, David Gibson 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.  The patch is a good idea, even though
> OBJECT(x)->y traditionally is not used (instead you assign OBJECT(x) to
> a different Object* variable).

Ok.. so are you prepared to give a Reviewed-by, or do I need to ask
someone else (Andreas?) to approve this as QOMishly correct?

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

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

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 15/09/2015 02:30, David Gibson wrote:
>>> I think the comment is wrong or at least inaccurate; it only
>>> applies to the external QOM interface.  The patch is a good
>>> idea, even though OBJECT(x)->y traditionally is not used
>>> (instead you assign OBJECT(x) to a different Object*
>>> variable).
> Ok.. so are you prepared to give a Reviewed-by, or do I need to
> ask someone else (Andreas?) to approve this as QOMishly correct?

I am, but as I said in another message I wonder if accessing the
parent field is actually necessary.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJV9/toAAoJEL/70l94x66DlgIH/3gVH53/bBLzUc77IfDbGlMZ
FIA94/uVb+HJWz29y0yv3V9RO+PpjhO9rQWUM3K+1TyH+Bu0st9HlGcxjhZVXpsH
So0IcVgBGFZry+3XyuIZ9TanpDcKo1cXSc5OZ/P7eBP/TviLcKshtfmn9WpgRWm7
DnVBFe1rSId7a8qn8BUeHyHtCLEwWcn8sOJla4w66PiGXVBG1p+mMU+lOXm6mQMD
sy3LNMRS2duhDyMncMF6tsTn03CesfiW+2b1U5U/Q3ZIFTLdOLFMTiBK8giTfd2J
FoePn1sZkF5SW4fPL8Gye1qbmnkpLUDMODHTUF3AI61Q11CAULFjxOkPK/QGEEI=
=lh5f
-----END PGP SIGNATURE-----

[Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors.

[David Gibson]

The dynamic reconfiguration (hotplug) code for the pseries machine type
uses a "DR connector" QOM object for each resource it will be possible
to hotplug.  Each of these is added to its owner using
    object_property_add_child(owner, "dr-connector[*], ...);

That works ok, mostly, but it means that the property indices are
arbitrary, depending on the order in which the connectors are constructed.
When we have both memory and cpu hotplug, the connectors will be under the
same parent (at least in the current drafts), meaning the indices don't
correspond to any meaningful ID.

It gets worse when large amounts of hotpluggable RAM is configured.  For
RAM, there's a DR connector object for every 256MB of potential memory.  So
if maxmem=2T, for example, there are 8192 objects under the same parent.

The QOM interfaces aren't really designed for this.  In particular
object_property_add() with [*] has O(n^2) time complexity (in the number of
existing children): first it has a linear search through array indices to
find a free slot, each of which is attempted to a recursive call to
object_property_add() with a specific [N].  Those calls are O(n) because
there's a linear search through all properties to check for duplicates.

By using a meaningful index value, which we already know is unique we can
avoid the [*] special behaviour.  That lets us reduce the total time for
creating the DR objects from O(n^3) to O(n^2).

O(n^2) is still kind of crappy, but it's enough to reduce the startup time
of qemu with maxmem=2T from ~20 minutes to ~4 seconds.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr_drc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 68e0c3e..2f95259 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -451,13 +451,16 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
 {
     sPAPRDRConnector *drc =
         SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
+    char *prop_name;
 
     g_assert(type);
 
     drc->type = type;
     drc->id = id;
-    object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
+    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc));
+    object_property_add_child(owner, prop_name, OBJECT(drc), NULL);
     object_property_set_bool(OBJECT(drc), true, "realized", NULL);
+    g_free(prop_name);
 
     /* human-readable name for a DRC to encode into the DT
      * description. this is mainly only used within a guest in place
-- 
2.4.3

Re: [Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors.

[Bharata B Rao]

On Mon, Sep 14, 2015 at 11:41:53AM +1000, David Gibson wrote:
> The dynamic reconfiguration (hotplug) code for the pseries machine type
> uses a "DR connector" QOM object for each resource it will be possible
> to hotplug.  Each of these is added to its owner using
>     object_property_add_child(owner, "dr-connector[*], ...);
> 
> That works ok, mostly, but it means that the property indices are
> arbitrary, depending on the order in which the connectors are constructed.
> When we have both memory and cpu hotplug, the connectors will be under the
> same parent (at least in the current drafts), meaning the indices don't
> correspond to any meaningful ID.
> 
> It gets worse when large amounts of hotpluggable RAM is configured.  For
> RAM, there's a DR connector object for every 256MB of potential memory.  So
> if maxmem=2T, for example, there are 8192 objects under the same parent.
> 
> The QOM interfaces aren't really designed for this.  In particular
> object_property_add() with [*] has O(n^2) time complexity (in the number of
> existing children): first it has a linear search through array indices to
> find a free slot, each of which is attempted to a recursive call to
> object_property_add() with a specific [N].  Those calls are O(n) because
> there's a linear search through all properties to check for duplicates.
> 
> By using a meaningful index value, which we already know is unique we can
> avoid the [*] special behaviour.  That lets us reduce the total time for
> creating the DR objects from O(n^3) to O(n^2).
> 
> O(n^2) is still kind of crappy, but it's enough to reduce the startup time
> of qemu with maxmem=2T from ~20 minutes to ~4 seconds.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>

This patch works correctly with both CPU and memory hotplug.

Regards,
Bharata.

Re: [Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors.

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

On Mon, Sep 14, 2015 at 09:37:16AM +0530, Bharata B Rao wrote:
> On Mon, Sep 14, 2015 at 11:41:53AM +1000, David Gibson wrote:
> > The dynamic reconfiguration (hotplug) code for the pseries machine type
> > uses a "DR connector" QOM object for each resource it will be possible
> > to hotplug.  Each of these is added to its owner using
> >     object_property_add_child(owner, "dr-connector[*], ...);
> > 
> > That works ok, mostly, but it means that the property indices are
> > arbitrary, depending on the order in which the connectors are constructed.
> > When we have both memory and cpu hotplug, the connectors will be under the
> > same parent (at least in the current drafts), meaning the indices don't
> > correspond to any meaningful ID.
> > 
> > It gets worse when large amounts of hotpluggable RAM is configured.  For
> > RAM, there's a DR connector object for every 256MB of potential memory.  So
> > if maxmem=2T, for example, there are 8192 objects under the same parent.
> > 
> > The QOM interfaces aren't really designed for this.  In particular
> > object_property_add() with [*] has O(n^2) time complexity (in the number of
> > existing children): first it has a linear search through array indices to
> > find a free slot, each of which is attempted to a recursive call to
> > object_property_add() with a specific [N].  Those calls are O(n) because
> > there's a linear search through all properties to check for duplicates.
> > 
> > By using a meaningful index value, which we already know is unique we can
> > avoid the [*] special behaviour.  That lets us reduce the total time for
> > creating the DR objects from O(n^3) to O(n^2).
> > 
> > O(n^2) is still kind of crappy, but it's enough to reduce the startup time
> > of qemu with maxmem=2T from ~20 minutes to ~4 seconds.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> This patch works correctly with both CPU and memory hotplug.

Care to send a Reviewed-by and/or Tested-by in that case?

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

Re: [Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors.

[Bharata B Rao]

On Mon, Sep 14, 2015 at 02:14:59PM +1000, David Gibson wrote:
> On Mon, Sep 14, 2015 at 09:37:16AM +0530, Bharata B Rao wrote:
> > On Mon, Sep 14, 2015 at 11:41:53AM +1000, David Gibson wrote:
> > > The dynamic reconfiguration (hotplug) code for the pseries machine type
> > > uses a "DR connector" QOM object for each resource it will be possible
> > > to hotplug.  Each of these is added to its owner using
> > >     object_property_add_child(owner, "dr-connector[*], ...);
> > > 
> > > That works ok, mostly, but it means that the property indices are
> > > arbitrary, depending on the order in which the connectors are constructed.
> > > When we have both memory and cpu hotplug, the connectors will be under the
> > > same parent (at least in the current drafts), meaning the indices don't
> > > correspond to any meaningful ID.
> > > 
> > > It gets worse when large amounts of hotpluggable RAM is configured.  For
> > > RAM, there's a DR connector object for every 256MB of potential memory.  So
> > > if maxmem=2T, for example, there are 8192 objects under the same parent.
> > > 
> > > The QOM interfaces aren't really designed for this.  In particular
> > > object_property_add() with [*] has O(n^2) time complexity (in the number of
> > > existing children): first it has a linear search through array indices to
> > > find a free slot, each of which is attempted to a recursive call to
> > > object_property_add() with a specific [N].  Those calls are O(n) because
> > > there's a linear search through all properties to check for duplicates.
> > > 
> > > By using a meaningful index value, which we already know is unique we can
> > > avoid the [*] special behaviour.  That lets us reduce the total time for
> > > creating the DR objects from O(n^3) to O(n^2).
> > > 
> > > O(n^2) is still kind of crappy, but it's enough to reduce the startup time
> > > of qemu with maxmem=2T from ~20 minutes to ~4 seconds.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > This patch works correctly with both CPU and memory hotplug.
> 
> Care to send a Reviewed-by and/or Tested-by in that case?

Sorry,

Tested-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Re: [Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors.

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

On Mon, Sep 14, 2015 at 10:11:50AM +0530, Bharata B Rao wrote:
> On Mon, Sep 14, 2015 at 02:14:59PM +1000, David Gibson wrote:
> > On Mon, Sep 14, 2015 at 09:37:16AM +0530, Bharata B Rao wrote:
> > > On Mon, Sep 14, 2015 at 11:41:53AM +1000, David Gibson wrote:
> > > > The dynamic reconfiguration (hotplug) code for the pseries machine type
> > > > uses a "DR connector" QOM object for each resource it will be possible
> > > > to hotplug.  Each of these is added to its owner using
> > > >     object_property_add_child(owner, "dr-connector[*], ...);
> > > > 
> > > > That works ok, mostly, but it means that the property indices are
> > > > arbitrary, depending on the order in which the connectors are constructed.
> > > > When we have both memory and cpu hotplug, the connectors will be under the
> > > > same parent (at least in the current drafts), meaning the indices don't
> > > > correspond to any meaningful ID.
> > > > 
> > > > It gets worse when large amounts of hotpluggable RAM is configured.  For
> > > > RAM, there's a DR connector object for every 256MB of potential memory.  So
> > > > if maxmem=2T, for example, there are 8192 objects under the same parent.
> > > > 
> > > > The QOM interfaces aren't really designed for this.  In particular
> > > > object_property_add() with [*] has O(n^2) time complexity (in the number of
> > > > existing children): first it has a linear search through array indices to
> > > > find a free slot, each of which is attempted to a recursive call to
> > > > object_property_add() with a specific [N].  Those calls are O(n) because
> > > > there's a linear search through all properties to check for duplicates.
> > > > 
> > > > By using a meaningful index value, which we already know is unique we can
> > > > avoid the [*] special behaviour.  That lets us reduce the total time for
> > > > creating the DR objects from O(n^3) to O(n^2).
> > > > 
> > > > O(n^2) is still kind of crappy, but it's enough to reduce the startup time
> > > > of qemu with maxmem=2T from ~20 minutes to ~4 seconds.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > 
> > > This patch works correctly with both CPU and memory hotplug.
> > 
> > Care to send a Reviewed-by and/or Tested-by in that case?
> 
> Sorry,
> 
> Tested-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

If you could send one for the cleanup in 1/2 as well, that would be nice.


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

Re: [Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors.

[Alexey Kardashevskiy]

On 09/14/2015 11:41 AM, David Gibson wrote:
> The dynamic reconfiguration (hotplug) code for the pseries machine type
> uses a "DR connector" QOM object for each resource it will be possible
> to hotplug.  Each of these is added to its owner using
>      object_property_add_child(owner, "dr-connector[*], ...);
>
> That works ok, mostly, but it means that the property indices are
> arbitrary, depending on the order in which the connectors are constructed.
> When we have both memory and cpu hotplug, the connectors will be under the
> same parent (at least in the current drafts), meaning the indices don't
> correspond to any meaningful ID.
>
> It gets worse when large amounts of hotpluggable RAM is configured.  For
> RAM, there's a DR connector object for every 256MB of potential memory.  So
> if maxmem=2T, for example, there are 8192 objects under the same parent.
>
> The QOM interfaces aren't really designed for this.  In particular
> object_property_add() with [*] has O(n^2) time complexity (in the number of
> existing children): first it has a linear search through array indices to
> find a free slot, each of which is attempted to a recursive call to
> object_property_add() with a specific [N].  Those calls are O(n) because
> there's a linear search through all properties to check for duplicates.
>
> By using a meaningful index value, which we already know is unique we can
> avoid the [*] special behaviour.  That lets us reduce the total time for
> creating the DR objects from O(n^3) to O(n^2).
>
> O(n^2) is still kind of crappy, but it's enough to reduce the startup time
> of qemu with maxmem=2T from ~20 minutes to ~4 seconds.

20 minutes sounds really cool :)

>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> ---
>   hw/ppc/spapr_drc.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 68e0c3e..2f95259 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -451,13 +451,16 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>   {
>       sPAPRDRConnector *drc =
>           SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
> +    char *prop_name;
>
>       g_assert(type);
>
>       drc->type = type;
>       drc->id = id;
> -    object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
> +    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc));
> +    object_property_add_child(owner, prop_name, OBJECT(drc), NULL);
>       object_property_set_bool(OBJECT(drc), true, "realized", NULL);
> +    g_free(prop_name);
>
>       /* human-readable name for a DRC to encode into the DT
>        * description. this is mainly only used within a guest in place
>


-- 
Alexey

Re: [Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors.

[Michael Roth]

Quoting David Gibson (2015-09-13 20:41:53)
> The dynamic reconfiguration (hotplug) code for the pseries machine type
> uses a "DR connector" QOM object for each resource it will be possible
> to hotplug.  Each of these is added to its owner using
>     object_property_add_child(owner, "dr-connector[*], ...);
> 
> That works ok, mostly, but it means that the property indices are
> arbitrary, depending on the order in which the connectors are constructed.
> When we have both memory and cpu hotplug, the connectors will be under the
> same parent (at least in the current drafts), meaning the indices don't
> correspond to any meaningful ID.
> 
> It gets worse when large amounts of hotpluggable RAM is configured.  For
> RAM, there's a DR connector object for every 256MB of potential memory.  So
> if maxmem=2T, for example, there are 8192 objects under the same parent.
> 
> The QOM interfaces aren't really designed for this.  In particular
> object_property_add() with [*] has O(n^2) time complexity (in the number of
> existing children): first it has a linear search through array indices to
> find a free slot, each of which is attempted to a recursive call to
> object_property_add() with a specific [N].  Those calls are O(n) because
> there's a linear search through all properties to check for duplicates.
> 
> By using a meaningful index value, which we already know is unique we can
> avoid the [*] special behaviour.  That lets us reduce the total time for
> creating the DR objects from O(n^3) to O(n^2).
> 
> O(n^2) is still kind of crappy, but it's enough to reduce the startup time
> of qemu with maxmem=2T from ~20 minutes to ~4 seconds.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>

This is the second patch I've seen that drops use of "[*]" for
performance reasons, but looking at the code I don't really see
any reason that logic can't be implemented in object_property_add()
in O(n) time. For instance I think it can be achieved by
storing/hashing the base string to an array of auto-incremented
indicies that we update whenever a child with the corresponding
<base_string>[n] format is added.

I wouldn't hold this real fix up for that though, and in fact the
use of DRC indexes make for much easier debugging anyway so I'd
probably still prefer this approach anyway.

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  hw/ppc/spapr_drc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 68e0c3e..2f95259 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -451,13 +451,16 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
>  {
>      sPAPRDRConnector *drc =
>          SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
> +    char *prop_name;
> 
>      g_assert(type);
> 
>      drc->type = type;
>      drc->id = id;
> -    object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
> +    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc));
> +    object_property_add_child(owner, prop_name, OBJECT(drc), NULL);
>      object_property_set_bool(OBJECT(drc), true, "realized", NULL);
> +    g_free(prop_name);
> 
>      /* human-readable name for a DRC to encode into the DT
>       * description. this is mainly only used within a guest in place
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors.

[Michael Roth]

Quoting Michael Roth (2015-09-14 23:03:00)
> Quoting David Gibson (2015-09-13 20:41:53)
> > The dynamic reconfiguration (hotplug) code for the pseries machine type
> > uses a "DR connector" QOM object for each resource it will be possible
> > to hotplug.  Each of these is added to its owner using
> >     object_property_add_child(owner, "dr-connector[*], ...);
> > 
> > That works ok, mostly, but it means that the property indices are
> > arbitrary, depending on the order in which the connectors are constructed.
> > When we have both memory and cpu hotplug, the connectors will be under the
> > same parent (at least in the current drafts), meaning the indices don't
> > correspond to any meaningful ID.
> > 
> > It gets worse when large amounts of hotpluggable RAM is configured.  For
> > RAM, there's a DR connector object for every 256MB of potential memory.  So
> > if maxmem=2T, for example, there are 8192 objects under the same parent.
> > 
> > The QOM interfaces aren't really designed for this.  In particular
> > object_property_add() with [*] has O(n^2) time complexity (in the number of
> > existing children): first it has a linear search through array indices to
> > find a free slot, each of which is attempted to a recursive call to
> > object_property_add() with a specific [N].  Those calls are O(n) because
> > there's a linear search through all properties to check for duplicates.
> > 
> > By using a meaningful index value, which we already know is unique we can
> > avoid the [*] special behaviour.  That lets us reduce the total time for
> > creating the DR objects from O(n^3) to O(n^2).
> > 
> > O(n^2) is still kind of crappy, but it's enough to reduce the startup time
> > of qemu with maxmem=2T from ~20 minutes to ~4 seconds.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> This is the second patch I've seen that drops use of "[*]" for
> performance reasons, but looking at the code I don't really see
> any reason that logic can't be implemented in object_property_add()
> in O(n) time. For instance I think it can be achieved by
> storing/hashing the base string to an array of auto-incremented
> indicies that we update whenever a child with the corresponding
> <base_string>[n] format is added.
> 
> I wouldn't hold this real fix up for that though, and in fact the
> use of DRC indexes make for much easier debugging anyway so I'd
> probably still prefer this approach anyway.

Well, along that line, one small nit: I think
%x would be a bit easier to correlate with the DRC indexes as we
use them elsewhere. Reviewed-by still stands though.

> 
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> > ---
> >  hw/ppc/spapr_drc.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 68e0c3e..2f95259 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -451,13 +451,16 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> >  {
> >      sPAPRDRConnector *drc =
> >          SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
> > +    char *prop_name;
> > 
> >      g_assert(type);
> > 
> >      drc->type = type;
> >      drc->id = id;
> > -    object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
> > +    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc));
> > +    object_property_add_child(owner, prop_name, OBJECT(drc), NULL);
> >      object_property_set_bool(OBJECT(drc), true, "realized", NULL);
> > +    g_free(prop_name);
> > 
> >      /* human-readable name for a DRC to encode into the DT
> >       * description. this is mainly only used within a guest in place
> > -- 
> > 2.4.3
> > 

Re: [Qemu-devel] [RFCv2 2/2] spapr: Don't use QOM [*] syntax for DR connectors.

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4043 bytes --]

On Mon, Sep 14, 2015 at 11:03:00PM -0500, Michael Roth wrote:
> Quoting David Gibson (2015-09-13 20:41:53)
> > The dynamic reconfiguration (hotplug) code for the pseries machine type
> > uses a "DR connector" QOM object for each resource it will be possible
> > to hotplug.  Each of these is added to its owner using
> >     object_property_add_child(owner, "dr-connector[*], ...);
> > 
> > That works ok, mostly, but it means that the property indices are
> > arbitrary, depending on the order in which the connectors are constructed.
> > When we have both memory and cpu hotplug, the connectors will be under the
> > same parent (at least in the current drafts), meaning the indices don't
> > correspond to any meaningful ID.
> > 
> > It gets worse when large amounts of hotpluggable RAM is configured.  For
> > RAM, there's a DR connector object for every 256MB of potential memory.  So
> > if maxmem=2T, for example, there are 8192 objects under the same parent.
> > 
> > The QOM interfaces aren't really designed for this.  In particular
> > object_property_add() with [*] has O(n^2) time complexity (in the number of
> > existing children): first it has a linear search through array indices to
> > find a free slot, each of which is attempted to a recursive call to
> > object_property_add() with a specific [N].  Those calls are O(n) because
> > there's a linear search through all properties to check for duplicates.
> > 
> > By using a meaningful index value, which we already know is unique we can
> > avoid the [*] special behaviour.  That lets us reduce the total time for
> > creating the DR objects from O(n^3) to O(n^2).
> > 
> > O(n^2) is still kind of crappy, but it's enough to reduce the startup time
> > of qemu with maxmem=2T from ~20 minutes to ~4 seconds.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> This is the second patch I've seen that drops use of "[*]" for
> performance reasons, but looking at the code I don't really see
> any reason that logic can't be implemented in object_property_add()
> in O(n) time. For instance I think it can be achieved by
> storing/hashing the base string to an array of auto-incremented
> indicies that we update whenever a child with the corresponding
> <base_string>[n] format is added.

Oh, there's no question that could be done.  The question is: is
thousands of QOM objects under a single parent a use case that we
actually want to build QOM for?

> I wouldn't hold this real fix up for that though, and in fact the
> use of DRC indexes make for much easier debugging anyway so I'd
> probably still prefer this approach anyway.
> 
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> > ---
> >  hw/ppc/spapr_drc.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 68e0c3e..2f95259 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -451,13 +451,16 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner,
> >  {
> >      sPAPRDRConnector *drc =
> >          SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR));
> > +    char *prop_name;
> > 
> >      g_assert(type);
> > 
> >      drc->type = type;
> >      drc->id = id;
> > -    object_property_add_child(owner, "dr-connector[*]", OBJECT(drc), NULL);
> > +    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc));
> > +    object_property_add_child(owner, prop_name, OBJECT(drc), NULL);
> >      object_property_set_bool(OBJECT(drc), true, "realized", NULL);
> > +    g_free(prop_name);
> > 
> >      /* human-readable name for a DRC to encode into the DT
> >       * description. this is mainly only used within a guest in place
> 

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