Re: [PATCH 6/6] spapr: Model DR connectors as simple objects

[David Gibson]

On Fri, Dec 18, 2020 at 11:34:00AM +0100, Greg Kurz wrote:
> Modeling DR connectors as individual devices raises some
> concerns, as already discussed a year ago in this thread:
> 
> https://patchew.org/QEMU/20191017205953.13122-1-cheloha@linux.vnet.ibm.com/
> 
> First, high maxmem settings creates too many DRC devices.
> This causes scalability issues. It severely increase boot
> time because the multiple traversals of the DRC list that
> are performed during machine setup are quadratic operations.
> This is directly related to the fact that DRCs are modeled
> as individual devices and added to the composition tree.
> 
> Second, DR connectors are really an internal concept of
> PAPR. They aren't something that the user or management
> layer can manipulate in any way. We already don't allow
> their creation with device_add by clearing user_creatable.
> 
> DR connectors don't even need to be modeled as actual
> devices since they don't sit in a bus. They just need
> to be associated to an 'owner' object and to have the
> equivalent of realize/unrealize functions.
> 
> Downgrade them to be simple objects. Convert the existing
> realize() and unrealize() to be methods of the DR connector
> base class. Also have the base class to inherit from the
> vmstate_if interface directly. The get_id() hook simply
> returns NULL, just as device_vmstate_if_get_id() does for
> devices that don't sit in a bus. The DR connector is no
> longer made a child object. This means that it must be
> explicitely freed when no longer needed. This is only
> required for PHBs and PCI bridges actually : have them to
> free the DRC with spapr_dr_connector_free() instead of
> object_unparent().
> 
> No longer add the DRCs to the QOM composition tree. Track
> them with a glib hash table using the global DRC index as
> the key instead. This makes traversal a linear operation.

I have some reservations about this one.  The main thing is that
attaching migration state to something that's not a device seems a bit
odd to me.  AFAICT exactly one other non-device implements
TYPE_VMSTATE_IF, and what it does isn't very clear to me.

As I might have mentioned to you I had a different idea for how to
address this problem: still use a TYPE_DEVICE, but have it manage a
whole array of DRCs as one unit, rather than just a single one.
Specifically I was thinking:

* one array per PCI bus (DRCs for each function on the bus)
* one array for each block of memory (so one for base memory, one for
  each DIMM)
* one array for all the cpus
* one array for all the PHBs

It has some disadvantages compared to your scheme: it still leaves
(less) devices which can't be user managed, which is a bit ugly.  On
the other hand, each of those arrays can reasonably be dense, so we
can use direct indexing rather than a hash table, which is a bit
nicer.

Thoughts?

> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  include/hw/ppc/spapr_drc.h |   8 +-
>  hw/ppc/spapr_drc.c         | 166 ++++++++++++++-----------------------
>  hw/ppc/spapr_pci.c         |   2 +-
>  3 files changed, 69 insertions(+), 107 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 8982927d5c24..a26aa8b9d4c3 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -170,7 +170,7 @@ typedef enum {
>  
>  typedef struct SpaprDrc {
>      /*< private >*/
> -    DeviceState parent;
> +    Object parent;
>  
>      uint32_t id;
>      Object *owner;
> @@ -193,7 +193,7 @@ struct SpaprMachineState;
>  
>  typedef struct SpaprDrcClass {
>      /*< private >*/
> -    DeviceClass parent;
> +    ObjectClass parent;
>      SpaprDrcState empty_state;
>      SpaprDrcState ready_state;
>  
> @@ -209,6 +209,9 @@ typedef struct SpaprDrcClass {
>  
>      int (*dt_populate)(SpaprDrc *drc, struct SpaprMachineState *spapr,
>                         void *fdt, int *fdt_start_offset, Error **errp);
> +
> +    void (*realize)(SpaprDrc *drc);
> +    void (*unrealize)(SpaprDrc *drc);
>  } SpaprDrcClass;
>  
>  typedef struct SpaprDrcPhysical {
> @@ -232,6 +235,7 @@ SpaprDrcType spapr_drc_type(SpaprDrc *drc);
>  
>  SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
>                                           uint32_t id);
> +void spapr_dr_connector_free(SpaprDrc *drc);
>  SpaprDrc *spapr_drc_by_index(uint32_t index);
>  SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id);
>  int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t 
> drc_type_mask);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8571d5bafe4e..e26763f8b5a4 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -27,7 +27,6 @@
>  #include "sysemu/reset.h"
>  #include "trace.h"
>  
> -#define DRC_CONTAINER_PATH "/dr-connector"
>  #define DRC_INDEX_TYPE_SHIFT 28
>  #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
>  
> @@ -503,65 +502,56 @@ static const VMStateDescription vmstate_spapr_drc = {
>      }
>  };
>  
> -static void drc_realize(DeviceState *d, Error **errp)
> +static GHashTable *drc_hash_table(void)
>  {
> -    SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
> -    Object *root_container;
> -    gchar *link_name;
> -    const char *child_name;
> +    static GHashTable *dht;
>  
> +    if (!dht) {
> +        dht = g_hash_table_new(NULL, NULL);
> +    }
> +
> +    return dht;
> +}
> +
> +
> +static void drc_realize(SpaprDrc *drc)
> +{
>      trace_spapr_drc_realize(spapr_drc_index(drc));
> -    /* NOTE: we do this as part of realize/unrealize due to the fact
> -     * that the guest will communicate with the DRC via RTAS calls
> -     * referencing the global DRC index. By unlinking the DRC
> -     * from DRC_CONTAINER_PATH/<drc_index> we effectively make it
> -     * inaccessible by the guest, since lookups rely on this path
> -     * existing in the composition tree
> -     */
> -    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> -    link_name = g_strdup_printf("%x", spapr_drc_index(drc));
> -    child_name = object_get_canonical_path_component(OBJECT(drc));
> -    trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
> -    object_property_add_alias(root_container, link_name,
> -                              drc->owner, child_name);
> -    g_free(link_name);
> +
> +    g_hash_table_insert(drc_hash_table(),
> +                        GUINT_TO_POINTER(spapr_drc_index(drc)), drc);
>      vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), 
> &vmstate_spapr_drc,
>                       drc);
>      trace_spapr_drc_realize_complete(spapr_drc_index(drc));
>  }
>  
> -static void drc_unrealize(DeviceState *d)
> +static void drc_unrealize(SpaprDrc *drc)
>  {
> -    SpaprDrc *drc = SPAPR_DR_CONNECTOR(d);
> -    Object *root_container;
> -    gchar *name;
> -
>      trace_spapr_drc_unrealize(spapr_drc_index(drc));
>      vmstate_unregister(VMSTATE_IF(drc), &vmstate_spapr_drc, drc);
> -    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> -    name = g_strdup_printf("%x", spapr_drc_index(drc));
> -    object_property_del(root_container, name);
> -    g_free(name);
> +    g_hash_table_remove(drc_hash_table(),
> +                        GUINT_TO_POINTER(spapr_drc_index(drc)));
>  }
>  
>  SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
>                                           uint32_t id)
>  {
>      SpaprDrc *drc = SPAPR_DR_CONNECTOR(object_new(type));
> -    char *prop_name;
>  
>      drc->id = id;
> -    drc->owner = owner;
> -    prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> -                                spapr_drc_index(drc));
> -    object_property_add_child(owner, prop_name, OBJECT(drc));
> -    object_unref(OBJECT(drc));
> -    qdev_realize(DEVICE(drc), NULL, NULL);
> -    g_free(prop_name);
> +    drc->owner = object_ref(owner);
> +    SPAPR_DR_CONNECTOR_GET_CLASS(drc)->realize(drc);
>  
>      return drc;
>  }
>  
> +void spapr_dr_connector_free(SpaprDrc *drc)
> +{
> +    SPAPR_DR_CONNECTOR_GET_CLASS(drc)->unrealize(drc);
> +    object_unref(drc->owner);
> +    object_unref(drc);
> +}
> +
>  static void spapr_dr_connector_instance_init(Object *obj)
>  {
>      SpaprDrc *drc = SPAPR_DR_CONNECTOR(obj);
> @@ -575,17 +565,19 @@ static void spapr_dr_connector_instance_init(Object 
> *obj)
>      drc->state = drck->empty_state;
>  }
>  
> +static char *drc_vmstate_if_get_id(VMStateIf *obj)
> +{
> +    return NULL;
> +}
> +
>  static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>  {
> -    DeviceClass *dk = DEVICE_CLASS(k);
> +    SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> +    VMStateIfClass *vc = VMSTATE_IF_CLASS(k);
>  
> -    dk->realize = drc_realize;
> -    dk->unrealize = drc_unrealize;
> -    /*
> -     * Reason: DR connector needs to be wired to either the machine or to a
> -     * PHB in spapr_dr_connector_new().
> -     */
> -    dk->user_creatable = false;
> +    drck->realize = drc_realize;
> +    drck->unrealize = drc_unrealize;
> +    vc->get_id = drc_vmstate_if_get_id;
>  }
>  
>  static bool drc_physical_needed(void *opaque)
> @@ -623,39 +615,32 @@ static void drc_physical_reset(void *opaque)
>      }
>  }
>  
> -static void realize_physical(DeviceState *d, Error **errp)
> +static void realize_physical(SpaprDrc *drc)
>  {
> -    SpaprDrcPhysical *drcp = SPAPR_DRC_PHYSICAL(d);
> -    Error *local_err = NULL;
> -
> -    drc_realize(d, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> +    SpaprDrcPhysical *drcp = SPAPR_DRC_PHYSICAL(drc);
>  
> +    drc_realize(drc);
>      vmstate_register(VMSTATE_IF(drcp),
>                       spapr_drc_index(SPAPR_DR_CONNECTOR(drcp)),
>                       &vmstate_spapr_drc_physical, drcp);
>      qemu_register_reset(drc_physical_reset, drcp);
>  }
>  
> -static void unrealize_physical(DeviceState *d)
> +static void unrealize_physical(SpaprDrc *drc)
>  {
> -    SpaprDrcPhysical *drcp = SPAPR_DRC_PHYSICAL(d);
> +    SpaprDrcPhysical *drcp = SPAPR_DRC_PHYSICAL(drc);
>  
> -    drc_unrealize(d);
> -    vmstate_unregister(VMSTATE_IF(drcp), &vmstate_spapr_drc_physical, drcp);
>      qemu_unregister_reset(drc_physical_reset, drcp);
> +    vmstate_unregister(VMSTATE_IF(drcp), &vmstate_spapr_drc_physical, drcp);
> +    drc_unrealize(drc);
>  }
>  
>  static void spapr_drc_physical_class_init(ObjectClass *k, void *data)
>  {
> -    DeviceClass *dk = DEVICE_CLASS(k);
>      SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
>  
> -    dk->realize = realize_physical;
> -    dk->unrealize = unrealize_physical;
> +    drck->realize = realize_physical;
> +    drck->unrealize = unrealize_physical;
>      drck->dr_entity_sense = physical_entity_sense;
>      drck->isolate = drc_isolate_physical;
>      drck->unisolate = drc_unisolate_physical;
> @@ -731,12 +716,16 @@ static void spapr_drc_pmem_class_init(ObjectClass *k, 
> void *data)
>  
>  static const TypeInfo spapr_dr_connector_info = {
>      .name          = TYPE_SPAPR_DR_CONNECTOR,
> -    .parent        = TYPE_DEVICE,
> +    .parent        = TYPE_OBJECT,
>      .instance_size = sizeof(SpaprDrc),
>      .instance_init = spapr_dr_connector_instance_init,
>      .class_size    = sizeof(SpaprDrcClass),
>      .class_init    = spapr_dr_connector_class_init,
>      .abstract      = true,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_VMSTATE_IF },
> +        { }
> +    },
>  };
>  
>  static const TypeInfo spapr_drc_physical_info = {
> @@ -789,14 +778,9 @@ static const TypeInfo spapr_drc_pmem_info = {
>  
>  SpaprDrc *spapr_drc_by_index(uint32_t index)
>  {
> -    Object *obj;
> -    gchar *name;
> -
> -    name = g_strdup_printf("%s/%x", DRC_CONTAINER_PATH, index);
> -    obj = object_resolve_path(name, NULL);
> -    g_free(name);
> -
> -    return !obj ? NULL : SPAPR_DR_CONNECTOR(obj);
> +    return
> +        SPAPR_DR_CONNECTOR(g_hash_table_lookup(drc_hash_table(),
> +                                               GUINT_TO_POINTER(index)));
>  }
>  
>  SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id)
> @@ -824,13 +808,12 @@ SpaprDrc *spapr_drc_by_id(const char *type, uint32_t id)
>   */
>  int spapr_dt_drc(void *fdt, int offset, Object *owner, uint32_t 
> drc_type_mask)
>  {
> -    Object *root_container;
> -    ObjectProperty *prop;
> -    ObjectPropertyIterator iter;
> +    GHashTableIter iter;
>      uint32_t drc_count = 0;
>      GArray *drc_indexes, *drc_power_domains;
>      GString *drc_names, *drc_types;
>      int ret;
> +    SpaprDrc *drc;
>  
>      /*
>       * This should really be only called once per node since it overwrites
> @@ -851,26 +834,12 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, 
> uint32_t drc_type_mask)
>      drc_names = g_string_set_size(g_string_new(NULL), sizeof(uint32_t));
>      drc_types = g_string_set_size(g_string_new(NULL), sizeof(uint32_t));
>  
> -    /* aliases for all DRConnector objects will be rooted in QOM
> -     * composition tree at DRC_CONTAINER_PATH
> -     */
> -    root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> -
> -    object_property_iter_init(&iter, root_container);
> -    while ((prop = object_property_iter_next(&iter))) {
> -        Object *obj;
> -        SpaprDrc *drc;
> +    g_hash_table_iter_init(&iter, drc_hash_table());
> +    while (g_hash_table_iter_next(&iter, NULL, (gpointer) &drc)) {
>          SpaprDrcClass *drck;
>          char *drc_name = NULL;
>          uint32_t drc_index, drc_power_domain;
>  
> -        if (!strstart(prop->type, "link<", NULL)) {
> -            continue;
> -        }
> -
> -        obj = object_property_get_link(root_container, prop->name,
> -                                       &error_abort);
> -        drc = SPAPR_DR_CONNECTOR(obj);
>          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
>          if (owner && (drc->owner != owner)) {
> @@ -951,23 +920,12 @@ out:
>  
>  void spapr_drc_reset_all(SpaprMachineState *spapr)
>  {
> -    Object *drc_container;
> -    ObjectProperty *prop;
> -    ObjectPropertyIterator iter;
> +    GHashTableIter iter;
> +    SpaprDrc *drc;
>  
> -    drc_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
>  restart:
> -    object_property_iter_init(&iter, drc_container);
> -    while ((prop = object_property_iter_next(&iter))) {
> -        SpaprDrc *drc;
> -
> -        if (!strstart(prop->type, "link<", NULL)) {
> -            continue;
> -        }
> -        drc = SPAPR_DR_CONNECTOR(object_property_get_link(drc_container,
> -                                                          prop->name,
> -                                                          &error_abort));
> -
> +    g_hash_table_iter_init(&iter, drc_hash_table());
> +    while (g_hash_table_iter_next(&iter, NULL, (gpointer) &drc)) {
>          /*
>           * This will complete any pending plug/unplug requests.
>           * In case of a unplugged PHB or PCI bridge, this will
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 76d7c91e9c64..ca0cca664e3c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1262,7 +1262,7 @@ static void remove_drcs(SpaprPhbState *phb, PCIBus *bus)
>          SpaprDrc *drc = drc_from_devfn(phb, chassis, i);
>  
>          if (drc) {
> -            object_unparent(OBJECT(drc));
> +            spapr_dr_connector_free(drc);
>          }
>      }
>  }

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