On Wed, 16 Oct 2019 at 14:50, Andrew Jones <drjo...@redhat.com> wrote:
>
> On Wed, Oct 16, 2019 at 02:24:50PM +0100, Beata Michalska wrote:
> > On Tue, 15 Oct 2019 at 12:56, Beata Michalska
> > <beata.michal...@linaro.org> wrote:
> > >
> > > On Tue, 15 Oct 2019 at 11:56, Andrew Jones <drjo...@redhat.com> wrote:
> > > >
> > > > On Tue, Oct 15, 2019 at 10:59:16AM +0100, Beata Michalska wrote:
> > > > > On Tue, 1 Oct 2019 at 14:04, Andrew Jones <drjo...@redhat.com> 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:
_______________________________________________________
---
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
