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 <address@hidden>
> > Cc: Bharata B Rao <address@hidden>
> 
> 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 <address@hidden>
> 
> > ---
> >  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
> >