Re: [PATCH 1/4] perf evsel: Add evsel__clone() function
Hi Jiri,
On Fri, Sep 18, 2020 at 10:31 PM Jiri Olsa wrote:
>
> On Wed, Sep 16, 2020 at 03:31:26PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > +struct evsel *evsel__clone(struct evsel *orig)
> > +{
> > + struct evsel *evsel;
> > + struct evsel_config_term *pos, *tmp;
> > +
> > + BUG_ON(orig->core.fd);
> > + BUG_ON(orig->counts);
> > + BUG_ON(orig->priv);
> > + BUG_ON(orig->per_pkg_mask);
> > +
> > + /* cannot handle BPF objects for now */
> > + if (orig->bpf_obj)
> > + return NULL;
> > +
> > + evsel = evsel__new(&orig->core.attr);
> > + if (evsel == NULL)
> > + return NULL;
> > +
> > + evsel->core.cpus = perf_cpu_map__get(orig->core.cpus);
> > + evsel->core.own_cpus = perf_cpu_map__get(orig->core.own_cpus);
> > + evsel->core.threads = perf_thread_map__get(orig->core.threads);
> > + evsel->core.nr_members = orig->core.nr_members;
> > + evsel->core.system_wide = orig->core.system_wide;
> > +
> > + if (orig->name)
> > + evsel->name = strdup(orig->name);
> > + if (orig->group_name)
> > + evsel->group_name = strdup(orig->group_name);
> > + if (orig->pmu_name)
> > + evsel->pmu_name = strdup(orig->pmu_name);
> > + if (orig->filter)
> > + evsel->filter = strdup(orig->filter);
>
> we should check those strdup results
ok.
>
> > + evsel->cgrp = cgroup__get(orig->cgrp);
> > + evsel->tp_format = orig->tp_format;
> > + evsel->handler = orig->handler;
> > + evsel->leader = orig->leader;
> > +
> > + evsel->max_events = orig->max_events;
> > + evsel->tool_event = orig->tool_event;
> > + evsel->unit = orig->unit;
> > + evsel->scale = orig->scale;
> > + evsel->snapshot = orig->snapshot;
> > + evsel->per_pkg = orig->per_pkg;
> > + evsel->percore = orig->percore;
> > + evsel->precise_max = orig->precise_max;
> > + evsel->use_uncore_alias = orig->use_uncore_alias;
> > + evsel->is_libpfm_event = orig->is_libpfm_event;
> > +
> > + evsel->exclude_GH = orig->exclude_GH;
> > + evsel->sample_read = orig->sample_read;
> > + evsel->auto_merge_stats = orig->auto_merge_stats;
> > + evsel->collect_stat = orig->collect_stat;
> > + evsel->weak_group = orig->weak_group;
>
> so all those evsel's members are possibly defined in parse time right?
> perhaps we should separate them in the struct? and make some note about
> evsel__clone function that new members should be considered for copy
> in evsel__close.. or something like that
Sounds good.
>
> > +
> > + list_for_each_entry(pos, &orig->config_terms, list) {
> > + tmp = malloc(sizeof(*tmp));
> > + if (tmp == NULL) {
> > + evsel__delete(evsel);
> > + evsel = NULL;
> > + break;
> > + }
> > +
> > + *tmp = *pos;
> > + if (tmp->free_str) {
> > + tmp->val.str = strdup(pos->val.str);
> > + if (tmp->val.str == NULL) {
> > + evsel__delete(evsel);
> > + evsel = NULL;
> > + free(tmp);
> > + break;
> > + }
> > + }
> > + list_add_tail(&tmp->list, &evsel->config_terms);
> > + }
>
> could this go in separate function? copy_terms
Will do.
Thanks
Namhyung
>
> > +
> > + return evsel;
> > +}
> > +
> > /*
> > * Returns pointer with encoded error via interface.
> > */
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index 35e3f6d66085..507c31d6a389 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -169,6 +169,7 @@ static inline struct evsel *evsel__new(struct
> > perf_event_attr *attr)
> > return evsel__new_idx(attr, 0);
> > }
> >
> > +struct evsel *evsel__clone(struct evsel *orig);
> > struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx);
> >
> > /*
> > --
> > 2.28.0.618.gf4bc123cb7-goog
> >
>
Re: [PATCH 1/4] perf evsel: Add evsel__clone() function
On Wed, Sep 16, 2020 at 03:31:26PM +0900, Namhyung Kim wrote:
SNIP
> +struct evsel *evsel__clone(struct evsel *orig)
> +{
> + struct evsel *evsel;
> + struct evsel_config_term *pos, *tmp;
> +
> + BUG_ON(orig->core.fd);
> + BUG_ON(orig->counts);
> + BUG_ON(orig->priv);
> + BUG_ON(orig->per_pkg_mask);
> +
> + /* cannot handle BPF objects for now */
> + if (orig->bpf_obj)
> + return NULL;
> +
> + evsel = evsel__new(&orig->core.attr);
> + if (evsel == NULL)
> + return NULL;
> +
> + evsel->core.cpus = perf_cpu_map__get(orig->core.cpus);
> + evsel->core.own_cpus = perf_cpu_map__get(orig->core.own_cpus);
> + evsel->core.threads = perf_thread_map__get(orig->core.threads);
> + evsel->core.nr_members = orig->core.nr_members;
> + evsel->core.system_wide = orig->core.system_wide;
> +
> + if (orig->name)
> + evsel->name = strdup(orig->name);
> + if (orig->group_name)
> + evsel->group_name = strdup(orig->group_name);
> + if (orig->pmu_name)
> + evsel->pmu_name = strdup(orig->pmu_name);
> + if (orig->filter)
> + evsel->filter = strdup(orig->filter);
we should check those strdup results
> + evsel->cgrp = cgroup__get(orig->cgrp);
> + evsel->tp_format = orig->tp_format;
> + evsel->handler = orig->handler;
> + evsel->leader = orig->leader;
> +
> + evsel->max_events = orig->max_events;
> + evsel->tool_event = orig->tool_event;
> + evsel->unit = orig->unit;
> + evsel->scale = orig->scale;
> + evsel->snapshot = orig->snapshot;
> + evsel->per_pkg = orig->per_pkg;
> + evsel->percore = orig->percore;
> + evsel->precise_max = orig->precise_max;
> + evsel->use_uncore_alias = orig->use_uncore_alias;
> + evsel->is_libpfm_event = orig->is_libpfm_event;
> +
> + evsel->exclude_GH = orig->exclude_GH;
> + evsel->sample_read = orig->sample_read;
> + evsel->auto_merge_stats = orig->auto_merge_stats;
> + evsel->collect_stat = orig->collect_stat;
> + evsel->weak_group = orig->weak_group;
so all those evsel's members are possibly defined in parse time right?
perhaps we should separate them in the struct? and make some note about
evsel__clone function that new members should be considered for copy
in evsel__close.. or something like that
> +
> + list_for_each_entry(pos, &orig->config_terms, list) {
> + tmp = malloc(sizeof(*tmp));
> + if (tmp == NULL) {
> + evsel__delete(evsel);
> + evsel = NULL;
> + break;
> + }
> +
> + *tmp = *pos;
> + if (tmp->free_str) {
> + tmp->val.str = strdup(pos->val.str);
> + if (tmp->val.str == NULL) {
> + evsel__delete(evsel);
> + evsel = NULL;
> + free(tmp);
> + break;
> + }
> + }
> + list_add_tail(&tmp->list, &evsel->config_terms);
> + }
could this go in separate function? copy_terms
thanks,
jirka
> +
> + return evsel;
> +}
> +
> /*
> * Returns pointer with encoded error via interface.
> */
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 35e3f6d66085..507c31d6a389 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -169,6 +169,7 @@ static inline struct evsel *evsel__new(struct
> perf_event_attr *attr)
> return evsel__new_idx(attr, 0);
> }
>
> +struct evsel *evsel__clone(struct evsel *orig);
> struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx);
>
> /*
> --
> 2.28.0.618.gf4bc123cb7-goog
>
[PATCH 1/4] perf evsel: Add evsel__clone() function
The evsel__clone() is to create an exactly same evsel from same
attributes. Note that metric events will be handled by later patch.
It will be used by perf stat to generate separate events for each
cgroup.
Signed-off-by: Namhyung Kim
---
tools/perf/util/evsel.c | 85 +
tools/perf/util/evsel.h | 1 +
2 files changed, 86 insertions(+)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index fd865002cbbd..29edef353036 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -331,6 +331,91 @@ struct evsel *evsel__new_cycles(bool precise)
goto out;
}
+/**
+ * evsel__clone - create a new evsel copied from @orig
+ * @orig: original evsel
+ *
+ * The assumption is that @orig is not configured nor opened yet.
+ * So we only care about the attributes that can be set while it's parsed.
+ */
+struct evsel *evsel__clone(struct evsel *orig)
+{
+ struct evsel *evsel;
+ struct evsel_config_term *pos, *tmp;
+
+ BUG_ON(orig->core.fd);
+ BUG_ON(orig->counts);
+ BUG_ON(orig->priv);
+ BUG_ON(orig->per_pkg_mask);
+
+ /* cannot handle BPF objects for now */
+ if (orig->bpf_obj)
+ return NULL;
+
+ evsel = evsel__new(&orig->core.attr);
+ if (evsel == NULL)
+ return NULL;
+
+ evsel->core.cpus = perf_cpu_map__get(orig->core.cpus);
+ evsel->core.own_cpus = perf_cpu_map__get(orig->core.own_cpus);
+ evsel->core.threads = perf_thread_map__get(orig->core.threads);
+ evsel->core.nr_members = orig->core.nr_members;
+ evsel->core.system_wide = orig->core.system_wide;
+
+ if (orig->name)
+ evsel->name = strdup(orig->name);
+ if (orig->group_name)
+ evsel->group_name = strdup(orig->group_name);
+ if (orig->pmu_name)
+ evsel->pmu_name = strdup(orig->pmu_name);
+ if (orig->filter)
+ evsel->filter = strdup(orig->filter);
+ evsel->cgrp = cgroup__get(orig->cgrp);
+ evsel->tp_format = orig->tp_format;
+ evsel->handler = orig->handler;
+ evsel->leader = orig->leader;
+
+ evsel->max_events = orig->max_events;
+ evsel->tool_event = orig->tool_event;
+ evsel->unit = orig->unit;
+ evsel->scale = orig->scale;
+ evsel->snapshot = orig->snapshot;
+ evsel->per_pkg = orig->per_pkg;
+ evsel->percore = orig->percore;
+ evsel->precise_max = orig->precise_max;
+ evsel->use_uncore_alias = orig->use_uncore_alias;
+ evsel->is_libpfm_event = orig->is_libpfm_event;
+
+ evsel->exclude_GH = orig->exclude_GH;
+ evsel->sample_read = orig->sample_read;
+ evsel->auto_merge_stats = orig->auto_merge_stats;
+ evsel->collect_stat = orig->collect_stat;
+ evsel->weak_group = orig->weak_group;
+
+ list_for_each_entry(pos, &orig->config_terms, list) {
+ tmp = malloc(sizeof(*tmp));
+ if (tmp == NULL) {
+ evsel__delete(evsel);
+ evsel = NULL;
+ break;
+ }
+
+ *tmp = *pos;
+ if (tmp->free_str) {
+ tmp->val.str = strdup(pos->val.str);
+ if (tmp->val.str == NULL) {
+ evsel__delete(evsel);
+ evsel = NULL;
+ free(tmp);
+ break;
+ }
+ }
+ list_add_tail(&tmp->list, &evsel->config_terms);
+ }
+
+ return evsel;
+}
+
/*
* Returns pointer with encoded error via interface.
*/
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 35e3f6d66085..507c31d6a389 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -169,6 +169,7 @@ static inline struct evsel *evsel__new(struct
perf_event_attr *attr)
return evsel__new_idx(attr, 0);
}
+struct evsel *evsel__clone(struct evsel *orig);
struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx);
/*
--
2.28.0.618.gf4bc123cb7-goog
Re: [PATCH 1/4] perf evsel: Add evsel__clone() function
Hi Jiri,
On Thu, Sep 10, 2020 at 5:59 PM Jiri Olsa wrote:
>
> On Tue, Sep 08, 2020 at 01:42:25PM +0900, Namhyung Kim wrote:
> > The evsel__clone() is to create an exactly same evsel from same
> > attributes. Note that metric events will be handled by later patch.
> >
> > It will be used by perf stat to generate separate events for each
> > cgroup.
> >
> > Signed-off-by: Namhyung Kim
> > ---
> > tools/perf/util/evsel.c | 57 +
> > tools/perf/util/evsel.h | 1 +
> > 2 files changed, 58 insertions(+)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index fd865002cbbd..4f50f9499973 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -331,6 +331,63 @@ struct evsel *evsel__new_cycles(bool precise)
> > goto out;
> > }
> >
> > +/**
> > + * evsel__clone - create a new evsel copied from @orig
> > + * @orig: original evsel
> > + *
> > + * The assumption is that @orig is not configured nor opened yet.
> > + * So we only care about the attributes that can be set while it's parsed.
> > + */
> > +struct evsel *evsel__clone(struct evsel *orig)
> > +{
> > + struct evsel *evsel;
> > + struct evsel_config_term *pos, *tmp;
> > +
> > + BUG_ON(orig->core.fd);
> > +
> > + evsel = evsel__new(&orig->core.attr);
> > + if (evsel == NULL)
> > + return NULL;
> > +
> > + *evsel = *orig;
>
> this seems wild ;-) I saw that assumption above,
> but I wonder we could add some check or zero/init
> the rest of the fields fields
Alternatively, we could only copy relevant fields explicitly.
Other fields will remain zero by evsel__new() above.
But it might be easy to be missed by future changes..
Thanks
Namhyung
Re: [PATCH 1/4] perf evsel: Add evsel__clone() function
On Tue, Sep 08, 2020 at 01:42:25PM +0900, Namhyung Kim wrote:
> The evsel__clone() is to create an exactly same evsel from same
> attributes. Note that metric events will be handled by later patch.
>
> It will be used by perf stat to generate separate events for each
> cgroup.
>
> Signed-off-by: Namhyung Kim
> ---
> tools/perf/util/evsel.c | 57 +
> tools/perf/util/evsel.h | 1 +
> 2 files changed, 58 insertions(+)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index fd865002cbbd..4f50f9499973 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -331,6 +331,63 @@ struct evsel *evsel__new_cycles(bool precise)
> goto out;
> }
>
> +/**
> + * evsel__clone - create a new evsel copied from @orig
> + * @orig: original evsel
> + *
> + * The assumption is that @orig is not configured nor opened yet.
> + * So we only care about the attributes that can be set while it's parsed.
> + */
> +struct evsel *evsel__clone(struct evsel *orig)
> +{
> + struct evsel *evsel;
> + struct evsel_config_term *pos, *tmp;
> +
> + BUG_ON(orig->core.fd);
> +
> + evsel = evsel__new(&orig->core.attr);
> + if (evsel == NULL)
> + return NULL;
> +
> + *evsel = *orig;
this seems wild ;-) I saw that assumption above,
but I wonder we could add some check or zero/init
the rest of the fields fields
jirka
[PATCH 1/4] perf evsel: Add evsel__clone() function
The evsel__clone() is to create an exactly same evsel from same
attributes. Note that metric events will be handled by later patch.
It will be used by perf stat to generate separate events for each
cgroup.
Signed-off-by: Namhyung Kim
---
tools/perf/util/evsel.c | 57 +
tools/perf/util/evsel.h | 1 +
2 files changed, 58 insertions(+)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index fd865002cbbd..4f50f9499973 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -331,6 +331,63 @@ struct evsel *evsel__new_cycles(bool precise)
goto out;
}
+/**
+ * evsel__clone - create a new evsel copied from @orig
+ * @orig: original evsel
+ *
+ * The assumption is that @orig is not configured nor opened yet.
+ * So we only care about the attributes that can be set while it's parsed.
+ */
+struct evsel *evsel__clone(struct evsel *orig)
+{
+ struct evsel *evsel;
+ struct evsel_config_term *pos, *tmp;
+
+ BUG_ON(orig->core.fd);
+
+ evsel = evsel__new(&orig->core.attr);
+ if (evsel == NULL)
+ return NULL;
+
+ *evsel = *orig;
+ evsel->evlist = NULL;
+ INIT_LIST_HEAD(&evsel->core.node);
+
+ evsel->core.cpus = perf_cpu_map__get(orig->core.cpus);
+ evsel->core.own_cpus = perf_cpu_map__get(orig->core.own_cpus);
+ evsel->core.threads = perf_thread_map__get(orig->core.threads);
+ if (orig->name)
+ evsel->name = strdup(orig->name);
+ if (orig->group_name)
+ evsel->group_name = strdup(orig->group_name);
+ if (orig->pmu_name)
+ evsel->pmu_name = strdup(orig->pmu_name);
+
+ INIT_LIST_HEAD(&evsel->config_terms);
+ list_for_each_entry(pos, &orig->config_terms, list) {
+ tmp = malloc(sizeof(*tmp));
+ if (tmp == NULL) {
+ evsel__delete(evsel);
+ evsel = NULL;
+ break;
+ }
+
+ *tmp = *pos;
+ if (tmp->free_str) {
+ tmp->val.str = strdup(pos->val.str);
+ if (tmp->val.str == NULL) {
+ evsel__delete(evsel);
+ evsel = NULL;
+ free(tmp);
+ break;
+ }
+ }
+ list_add_tail(&tmp->list, &evsel->config_terms);
+ }
+
+ return evsel;
+}
+
/*
* Returns pointer with encoded error via interface.
*/
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 35e3f6d66085..507c31d6a389 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -169,6 +169,7 @@ static inline struct evsel *evsel__new(struct
perf_event_attr *attr)
return evsel__new_idx(attr, 0);
}
+struct evsel *evsel__clone(struct evsel *orig);
struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx);
/*
--
2.28.0.526.ge36021eeef-goog
