Re: [PATCH v5 1/9] target/arm/monitor: Introduce qmp_query_cpu_model_expansion

[Andrew Jones]

On Wed, Oct 16, 2019 at 04:16:57PM +0100, Beata Michalska wrote:
> On Wed, 16 Oct 2019 at 14:50, Andrew Jones <address@hidden> wrote:
> >
> > On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote:
> > > On Tue, 15 Oct 2019 at 12:56, Beata Michalska
> > > <address@hidden> wrote:
> > > >
> > > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones <address@hidden> wrote:
> > > > >
> > > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> > > > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <address@hidden> wrote:
> > > > > > > +
> > > > > > > +    obj = object_new(object_class_get_name(oc));
> > > > > > > +
> > > > > > > +    if (qdict_in) {
> > > > > > > +        Visitor *visitor;
> > > > > > > +        Error *err = NULL;
> > > > > > > +
> > > > > > > +        visitor = qobject_input_visitor_new(model->props);
> > > > > > > +        visit_start_struct(visitor, NULL, NULL, 0, &err);
> > > > > > > +        if (err) {
> > > > > > > +            object_unref(obj);
> > > > > >
> > > > > > Shouldn't we free the 'visitor' here as well ?
> > > > >
> > > > > Yes. Good catch. So we also need to fix
> > > > > target/s390x/cpu_models.c:cpu_model_from_info(), which has the same
> > > > > construction (the construction from which I derived this)
> > > > >
> > > > > >
> > > > > > > +            error_propagate(errp, err);
> > > > > > > +            return NULL;
> > > > > > > +        }
> > > > > > > +
> > > > >
> > > > > What about the rest of the patch? With that fixed for v6 can I
> > > > > add your r-b?
> > > > >
> > > >
> > > > I still got this feeling that we could optimize that a bit - which I'm
> > > > currently on, so hopefully I'll be able to add more comments soon if
> > > > that proves to be the case.
> > > >
> > > > BR
> > > > Beata
> > >
> > > I think there are few options that might be considered though the gain
> > > is not huge .. but it's always smth:
> > >
> > > > +CpuModelExpansionInfo 
> > > > *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> > > > +                                                     CpuModelInfo 
> > > > *model,
> > > > +                                                     Error **errp)
> > > > +{
> > > > +    CpuModelExpansionInfo *expansion_info;
> > > > +    const QDict *qdict_in = NULL;
> > > > +    QDict *qdict_out;
> > > > +    ObjectClass *oc;
> > > > +    Object *obj;
> > > > +    const char *name;
> > > > +    int i;
> > > > +
> > > > +    if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
> > > > +        error_setg(errp, "The requested expansion type is not 
> > > > supported");
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    if (!kvm_enabled() && !strcmp(model->name, "host")) {
> > > > +        error_setg(errp, "The CPU type '%s' requires KVM", 
> > > > model->name);
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
> > > > +    if (!oc) {
> > > > +        error_setg(errp, "The CPU type '%s' is not a recognized ARM 
> > > > CPU type",
> > > > +                   model->name);
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    if (kvm_enabled()) {
> > > > +        const char *cpu_type = current_machine->cpu_type;
> > > > +        int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> > > > +        bool supported = false;
> > > > +
> > > > +        if (!strcmp(model->name, "host") || !strcmp(model->name, 
> > > > "max")) {
> > > > +            /* These are kvmarm's recommended cpu types */
> > > > +            supported = true;
> > > > +        } else if (strlen(model->name) == len &&
> > > > +                   !strncmp(model->name, cpu_type, len)) {
> > > > +            /* KVM is enabled and we're using this type, so it works. 
> > > > */
> > > > +            supported = true;
> > > > +        }
> > > > +        if (!supported) {
> > > > +            error_setg(errp, "We cannot guarantee the CPU type '%s' 
> > > > works "
> > > > +                             "with KVM on this host", model->name);
> > > > +            return NULL;
> > > > +        }
> > > > +    }
> > > > +
> > >
> > > The above section can be slightly reduced and rearranged - preferably
> > > moved to a separate function
> > > -> get_cpu_model (...) ?
> > >
> > > * You can check the 'host' model first and then validate the accelerator 
> > > ->
> > >     if ( !strcmp(model->name, "host")
> > >         if (!kvm_enabled())
> > >             log_error & leave
> > >        else
> > >           goto cpu_class_by_name /*cpu_class_by_name moved after the
> > > final model check @see below */
> > >
> > > * the kvm_enabled section can be than slightly improved (dropping the
> > > second compare against 'host')
> > >
> > >       if (kvm_enabled() && strcmp(model->name, "max") {
> > >            /*Validate the current_machine->cpu_type against the
> > > model->name and report error case mismatch
> > >           /* otherwise just fall through */
> > >       }
> > >  * cpu_class_by_name moved here ...
> > > > +    if (model->props) {
> > > MInor: the CPUModelInfo seems to have dedicated field for that
> > > verification -> has_props
> > >
> > > > +        qdict_in = qobject_to(QDict, model->props);
> > > > +        if (!qdict_in) {
> > > > +            error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", 
> > > > "dict");
> > > > +            return NULL;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    obj = object_new(object_class_get_name(oc));
> > > > +
> > > > +    if (qdict_in) {
> > > > +        Visitor *visitor;
> > > > +        Error *err = NULL;
> > > > +
> > > > +        visitor = qobject_input_visitor_new(model->props);
> > > > +        visit_start_struct(visitor, NULL, NULL, 0, &err);
> > > > +        if (err) {
> > > > +            object_unref(obj);
> > > > +            error_propagate(errp, err);
> > > > +            return NULL;
> > > > +        }
> > > > +
> > > > +        i = 0;
> > > > +        while ((name = cpu_model_advertised_features[i++]) != NULL) {
> > > > +            if (qdict_get(qdict_in, name)) {
> > > > +                object_property_set(obj, visitor, name, &err);
> > > > +                if (err) {
> > > > +                    break;
> > > > +                }
> > > > +            }
> > > > +        }
> > > > +
> > > > +        if (!err) {
> > > > +            visit_check_struct(visitor, &err);
> > > > +        }
> > > > +        visit_end_struct(visitor, NULL);
> > > > +        visit_free(visitor);
> > > > +        if (err) {
> > > > +            object_unref(obj);
> > > > +            error_propagate(errp, err);
> > > > +            return NULL;
> > > > +        }
> > > > +    }
> > >
> > > The both >> if (err) << blocks could be extracted and moved at the end
> > > of the function
> > > to mark a 'cleanup section'  and both here and few lines before
> > > (with the visit_start_struct failure) could use goto.
> > > Easier to maintain and to make sure we make the proper cleanup in any 
> > > case.
> > >
> > > > +
> > > > +    expansion_info = g_new0(CpuModelExpansionInfo, 1);
> > > > +    expansion_info->model = g_malloc0(sizeof(*expansion_info->model));
> > > > +    expansion_info->model->name = g_strdup(model->name);
> > > > +
> > > > +    qdict_out = qdict_new();
> > > > +
> > > > +    i = 0;
> > > > +    while ((name = cpu_model_advertised_features[i++]) != NULL) {
> > > > +        ObjectProperty *prop = object_property_find(obj, name, NULL);
> > > > +        if (prop) {
> > > > +            Error *err = NULL;
> > > > +            QObject *value;
> > > > +
> > > > +            assert(prop->get);
> > > > +            value = object_property_get_qobject(obj, name, &err);
> > > > +            assert(!err);
> > > > +
> > > > +            qdict_put_obj(qdict_out, name, value);
> > > > +        }
> > > > +    }
> > > > +
> > >
> > > This could be merged with the first iteration over the features,
> > > smth like:
> > >
> > >     while () {
> > >         if ((value = qdict_get(qdict_in, name))) {
> > >             object_property_set ...
> > >            if (!err)
> > >                qobject_ref(value) /* we have the weak reference */
> > >             else
> > >                 break;
> > >         } else {
> > >              value = object_property_get_qobject()
> > >         }
> > >         if (value) qdict_put_object(qdict_out, name, value)
> > >     }
> > >
> > > This way you iterate over the table once and you do not query
> > > for the same property twice by getting the value from the qdict_in.
> > > If the value is not acceptable we will fail either way so should be all 
> > > good.
> > >
> > >
> > > > +    if (!qdict_size(qdict_out)) {
> > > > +        qobject_unref(qdict_out);
> > > > +    } else {
> > > > +        expansion_info->model->props = QOBJECT(qdict_out);
> > > > +        expansion_info->model->has_props = true;
> > > > +    }
> > > > +
> > > > +    object_unref(obj);
> > > > +
> > > > +    return expansion_info;
> > >
> > > Mentioned earlier cleanup section:
> > > cleanup:
> > >    object_unref(obj);
> > >    qobject_unref(qdict_out) ; /* if single loop is used */
> > >    error_propagate(errp,err);
> > >    return NULL;
> > >
> > > > +}
> > > > --
> > > > 2.20.1
> > > >
> > >
> > > Hope I haven't missed anything.
> > > What do you think ?
> > >
> >
> > I think you need to post an entire function that incorporates all the
> > proposed changes, or at least a diff that I can apply in order to get
> > the entirely changed function. I also think that it's fine the way
> > it is, so it would take a justification stronger than a potential
> > micro optimization to get me to change it.
> >
> 
> The numbers I can pull out of it are not thrilling and this is not
> on a fast path so I will not be pushing for changes.
> Though extracting the clean-up might be worth considering -
> for improved maintenance.
> 
> For a reference though:

It doesn't apply for me - even after fixing up the damager your mailer did
to it. I'd be surprised if it worked though. Merging the two loops over
features makes the output generation depend on the caller providing input.
Did you try the arm-cpu-features test with these changes?

drew

> 
> _______________________________________________________
> 
> ---
>  target/arm/monitor.c | 100 
> +++++++++++++++++++++++++--------------------------
>  1 file changed, 50 insertions(+), 50 deletions(-)
> 
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index edca8aa..0d6bd42 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -112,17 +112,40 @@ CpuModelExpansionInfo
> *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>      Object *obj;
>      const char *name;
>      int i;
> +    Error *err = NULL;
> 
>      if (type != CPU_MODEL_EXPANSION_TYPE_FULL) {
>          error_setg(errp, "The requested expansion type is not supported");
>          return NULL;
>      }
> 
> -    if (!kvm_enabled() && !strcmp(model->name, "host")) {
> -        error_setg(errp, "The CPU type '%s' requires KVM", model->name);
> -        return NULL;
> +    /* CPU type => 'host' */
> +    if (!strcmp(model->name, "host")) {
> +        if (!kvm_enabled()) {
> +            error_setg(errp, "The CPU type '%s' requires KVM", model->name);
> +            return NULL;
> +        } else {
> +            goto valid;
> +        }
> +    }
> +
> +
> +    /* Case when KVM is enabled and the model is a specific cpu model ... */
> +    if (kvm_enabled() && strcmp(model->name, "max")) {
> +            const char *cpu_type = current_machine->cpu_type;
> +            int len = strlen(cpu_type) - strlen("-" TYPE_ARM_CPU);
> +
> +            if (strlen(model->name) == len
> +             && !strncmp(cpu_type, model->name, len)) {
> +                error_setg(errp, "We cannot guarantee the CPU type '%s' 
> works "
> +                        "with KVM on this host", model->name);
> +                return NULL;
> +            }
> +
>      }
> 
> +valid:
> +
>      oc = cpu_class_by_name(TYPE_ARM_CPU, model->name);
>      if (!oc) {
>          error_setg(errp, "The CPU type '%s' is not a recognized ARM CPU 
> type",
> @@ -130,25 +153,6 @@ CpuModelExpansionInfo
> *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>          return NULL;
>      }
> 
> -    if (kvm_enabled()) {
> -        const char *cpu_type = current_machine->cpu_type;
> -        int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> -        bool supported = false;
> -
> -        if (!strcmp(model->name, "host") || !strcmp(model->name, "max")) {
> -            /* These are kvmarm's recommended cpu types */
> -            supported = true;
> -        } else if (strlen(model->name) == len &&
> -                   !strncmp(model->name, cpu_type, len)) {
> -            /* KVM is enabled and we're using this type, so it works. */
> -            supported = true;
> -        }
> -        if (!supported) {
> -            error_setg(errp, "We cannot guarantee the CPU type '%s' works "
> -                             "with KVM on this host", model->name);
> -            return NULL;
> -        }
> -    }
> 
>      if (model->props) {
>          qdict_in = qobject_to(QDict, model->props);
> @@ -159,62 +163,52 @@ CpuModelExpansionInfo
> *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>      }
> 
>      obj = object_new(object_class_get_name(oc));
> +    qdict_out = qdict_new();
> 
>      if (qdict_in) {
>          Visitor *visitor;
> -        Error *err = NULL;
> +        QObject *value;
> 
>          visitor = qobject_input_visitor_new(model->props);
>          visit_start_struct(visitor, NULL, NULL, 0, &err);
>          if (err) {
> -            object_unref(obj);
> -            error_propagate(errp, err);
> -            return NULL;
> +            visit_free(visitor);
> +            goto cleanup;
>          }
> -
>          i = 0;
>          while ((name = cpu_model_advertised_features[i++]) != NULL) {
> -            if (qdict_get(qdict_in, name)) {
> +            value = qdict_get(qdict_in, name);
> +            if (value) {
>                  object_property_set(obj, visitor, name, &err);
> -                if (err) {
> +                if (!err) {
> +                    qobject_ref(value);
> +                } else {
>                      break;
>                  }
> +
> +            } else {
> +               value = object_property_get_qobject(obj, name, &err);
>              }
> -        }
> 
> +            if (value) {
> +                qdict_put_obj(qdict_out, name, value);
> +            }
> +        }
>          if (!err) {
>              visit_check_struct(visitor, &err);
>          }
>          visit_end_struct(visitor, NULL);
>          visit_free(visitor);
>          if (err) {
> -            object_unref(obj);
> -            error_propagate(errp, err);
> -            return NULL;
> +            goto cleanup;
>          }
> +
>      }
> 
>      expansion_info = g_new0(CpuModelExpansionInfo, 1);
>      expansion_info->model = g_malloc0(sizeof(*expansion_info->model));
>      expansion_info->model->name = g_strdup(model->name);
> 
> -    qdict_out = qdict_new();
> -
> -    i = 0;
> -    while ((name = cpu_model_advertised_features[i++]) != NULL) {
> -        ObjectProperty *prop = object_property_find(obj, name, NULL);
> -        if (prop) {
> -            Error *err = NULL;
> -            QObject *value;
> -
> -            assert(prop->get);
> -            value = object_property_get_qobject(obj, name, &err);
> -            assert(!err);
> -
> -            qdict_put_obj(qdict_out, name, value);
> -        }
> -    }
> -
>      if (!qdict_size(qdict_out)) {
>          qobject_unref(qdict_out);
>      } else {
> @@ -225,4 +219,10 @@ CpuModelExpansionInfo
> *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
>      object_unref(obj);
> 
>      return expansion_info;
> +
> +cleanup:
> +    object_unref(obj);
> +    qobject_unref(qdict_out);
> +    error_propagate(errp, err);
> +    return NULL;
>  }
> -- 
> 2.7.4
> 
> BR
> Beata
> 
> > Thanks,
> > drew
>