| Message ID | 20191024190202.109403-7-irogers@google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Improvements to memory usage by parse events | expand |
On Thu, Oct 24, 2019 at 12:01:59PM -0700, Ian Rogers wrote: > If parsing fails then destructors are ran to clean the up the stack. > Rename the head union member to make the term and evlist use cases more > distinct, this simplifies matching the correct destructor. nice did not know about this.. looks like it's been in bison for some time, right? > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/parse-events.y | 69 +++++++++++++++++++++++----------- > 1 file changed, 48 insertions(+), 21 deletions(-) > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > index 545ab7cefc20..4725b14b9db4 100644 > --- a/tools/perf/util/parse-events.y > +++ b/tools/perf/util/parse-events.y > @@ -12,6 +12,7 @@ > #include <stdio.h> > #include <linux/compiler.h> > #include <linux/types.h> > +#include <linux/zalloc.h> > #include "pmu.h" > #include "evsel.h" > #include "parse-events.h" > @@ -37,6 +38,25 @@ static struct list_head* alloc_list() > return list; > } > > +static void free_list_evsel(struct list_head* list_evsel) > +{ > + struct perf_evsel *pos, *tmp; > + > + list_for_each_entry_safe(pos, tmp, list_evsel, node) { > + list_del_init(&pos->node); > + perf_evsel__delete(pos); > + } > + free(list_evsel); I think you need to iterate 'struct evsel' in here, not 'struct perf_evsel' should be: struct evsel *evsel, *tmp; list_for_each_entry_safe(evsel, tmp, list_evsel, core.node) { list_del_init(&evsel->core.node); evsel__delete(evsel); } thanks, jirka > +} > + > +static void free_term(struct parse_events_term *term) > +{ > + if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) > + free(term->val.str); > + zfree(&term->array.ranges); > + free(term); > +} > + > static void inc_group_count(struct list_head *list, > struct parse_events_state *parse_state) > { > @@ -66,6 +86,7 @@ static void inc_group_count(struct list_head *list, > %type <num> PE_VALUE_SYM_TOOL > %type <num> PE_RAW > %type <num> PE_TERM > +%type <num> value_sym > %type <str> PE_NAME > %type <str> PE_BPF_OBJECT > %type <str> PE_BPF_SOURCE > @@ -76,37 +97,43 @@ static void inc_group_count(struct list_head *list, > %type <str> PE_EVENT_NAME > %type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT > %type <str> PE_DRV_CFG_TERM > -%type <num> value_sym > -%type <head> event_config > -%type <head> opt_event_config > -%type <head> opt_pmu_config > +%destructor { free ($$); } <str> > %type <term> event_term > -%type <head> event_pmu > -%type <head> event_legacy_symbol > -%type <head> event_legacy_cache > -%type <head> event_legacy_mem > -%type <head> event_legacy_tracepoint > +%destructor { free_term ($$); } <term> > +%type <list_terms> event_config > +%type <list_terms> opt_event_config > +%type <list_terms> opt_pmu_config > +%destructor { parse_events_terms__delete ($$); } <list_terms> > +%type <list_evsel> event_pmu > +%type <list_evsel> event_legacy_symbol > +%type <list_evsel> event_legacy_cache > +%type <list_evsel> event_legacy_mem > +%type <list_evsel> event_legacy_tracepoint > +%type <list_evsel> event_legacy_numeric > +%type <list_evsel> event_legacy_raw > +%type <list_evsel> event_bpf_file > +%type <list_evsel> event_def > +%type <list_evsel> event_mod > +%type <list_evsel> event_name > +%type <list_evsel> event > +%type <list_evsel> events > +%type <list_evsel> group_def > +%type <list_evsel> group > +%type <list_evsel> groups > +%destructor { free_list_evsel ($$); } <list_evsel> > %type <tracepoint_name> tracepoint_name > -%type <head> event_legacy_numeric > -%type <head> event_legacy_raw > -%type <head> event_bpf_file > -%type <head> event_def > -%type <head> event_mod > -%type <head> event_name > -%type <head> event > -%type <head> events > -%type <head> group_def > -%type <head> group > -%type <head> groups > +%destructor { free ($$.sys); free ($$.event); } <tracepoint_name> > %type <array> array > %type <array> array_term > %type <array> array_terms > +%destructor { free ($$.ranges); } <array> > > %union > { > char *str; > u64 num; > - struct list_head *head; > + struct list_head *list_evsel; > + struct list_head *list_terms; > struct parse_events_term *term; > struct tracepoint_name { > char *sys; > -- > 2.23.0.866.gb869b98d4c-goog >
On Fri, Oct 25, 2019 at 1:27 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Oct 24, 2019 at 12:01:59PM -0700, Ian Rogers wrote: > > If parsing fails then destructors are ran to clean the up the stack. > > Rename the head union member to make the term and evlist use cases more > > distinct, this simplifies matching the correct destructor. > > nice did not know about this.. looks like it's been in bison for some time, right? Looks like it wasn't in Bison 1 but in Bison 2, we're at Bison 3 and Bison 2 is > 14 years old: https://web.archive.org/web/20050924004158/http://www.gnu.org/software/bison/manual/html_mono/bison.html#Destructor-Decl > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/parse-events.y | 69 +++++++++++++++++++++++----------- > > 1 file changed, 48 insertions(+), 21 deletions(-) > > > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > > index 545ab7cefc20..4725b14b9db4 100644 > > --- a/tools/perf/util/parse-events.y > > +++ b/tools/perf/util/parse-events.y > > @@ -12,6 +12,7 @@ > > #include <stdio.h> > > #include <linux/compiler.h> > > #include <linux/types.h> > > +#include <linux/zalloc.h> > > #include "pmu.h" > > #include "evsel.h" > > #include "parse-events.h" > > @@ -37,6 +38,25 @@ static struct list_head* alloc_list() > > return list; > > } > > > > +static void free_list_evsel(struct list_head* list_evsel) > > +{ > > + struct perf_evsel *pos, *tmp; > > + > > + list_for_each_entry_safe(pos, tmp, list_evsel, node) { > > + list_del_init(&pos->node); > > + perf_evsel__delete(pos); > > + } > > + free(list_evsel); > > I think you need to iterate 'struct evsel' in here, not 'struct perf_evsel' > > should be: > > struct evsel *evsel, *tmp; > > list_for_each_entry_safe(evsel, tmp, list_evsel, core.node) { > list_del_init(&evsel->core.node); > evsel__delete(evsel); > } Thanks, I'll address this. Ian > thanks, > jirka > > > +} > > + > > +static void free_term(struct parse_events_term *term) > > +{ > > + if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) > > + free(term->val.str); > > + zfree(&term->array.ranges); > > + free(term); > > +} > > + > > static void inc_group_count(struct list_head *list, > > struct parse_events_state *parse_state) > > { > > @@ -66,6 +86,7 @@ static void inc_group_count(struct list_head *list, > > %type <num> PE_VALUE_SYM_TOOL > > %type <num> PE_RAW > > %type <num> PE_TERM > > +%type <num> value_sym > > %type <str> PE_NAME > > %type <str> PE_BPF_OBJECT > > %type <str> PE_BPF_SOURCE > > @@ -76,37 +97,43 @@ static void inc_group_count(struct list_head *list, > > %type <str> PE_EVENT_NAME > > %type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT > > %type <str> PE_DRV_CFG_TERM > > -%type <num> value_sym > > -%type <head> event_config > > -%type <head> opt_event_config > > -%type <head> opt_pmu_config > > +%destructor { free ($$); } <str> > > %type <term> event_term > > -%type <head> event_pmu > > -%type <head> event_legacy_symbol > > -%type <head> event_legacy_cache > > -%type <head> event_legacy_mem > > -%type <head> event_legacy_tracepoint > > +%destructor { free_term ($$); } <term> > > +%type <list_terms> event_config > > +%type <list_terms> opt_event_config > > +%type <list_terms> opt_pmu_config > > +%destructor { parse_events_terms__delete ($$); } <list_terms> > > +%type <list_evsel> event_pmu > > +%type <list_evsel> event_legacy_symbol > > +%type <list_evsel> event_legacy_cache > > +%type <list_evsel> event_legacy_mem > > +%type <list_evsel> event_legacy_tracepoint > > +%type <list_evsel> event_legacy_numeric > > +%type <list_evsel> event_legacy_raw > > +%type <list_evsel> event_bpf_file > > +%type <list_evsel> event_def > > +%type <list_evsel> event_mod > > +%type <list_evsel> event_name > > +%type <list_evsel> event > > +%type <list_evsel> events > > +%type <list_evsel> group_def > > +%type <list_evsel> group > > +%type <list_evsel> groups > > +%destructor { free_list_evsel ($$); } <list_evsel> > > %type <tracepoint_name> tracepoint_name > > -%type <head> event_legacy_numeric > > -%type <head> event_legacy_raw > > -%type <head> event_bpf_file > > -%type <head> event_def > > -%type <head> event_mod > > -%type <head> event_name > > -%type <head> event > > -%type <head> events > > -%type <head> group_def > > -%type <head> group > > -%type <head> groups > > +%destructor { free ($$.sys); free ($$.event); } <tracepoint_name> > > %type <array> array > > %type <array> array_term > > %type <array> array_terms > > +%destructor { free ($$.ranges); } <array> > > > > %union > > { > > char *str; > > u64 num; > > - struct list_head *head; > > + struct list_head *list_evsel; > > + struct list_head *list_terms; > > struct parse_events_term *term; > > struct tracepoint_name { > > char *sys; > > -- > > 2.23.0.866.gb869b98d4c-goog > > >
On Fri, Oct 25, 2019 at 09:08:34AM -0700, Ian Rogers wrote: > On Fri, Oct 25, 2019 at 1:27 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Thu, Oct 24, 2019 at 12:01:59PM -0700, Ian Rogers wrote: > > > If parsing fails then destructors are ran to clean the up the stack. > > > Rename the head union member to make the term and evlist use cases more > > > distinct, this simplifies matching the correct destructor. > > > > nice did not know about this.. looks like it's been in bison for some time, right? > > Looks like it wasn't in Bison 1 but in Bison 2, we're at Bison 3 and > Bison 2 is > 14 years old: > https://web.archive.org/web/20050924004158/http://www.gnu.org/software/bison/manual/html_mono/bison.html#Destructor-Decl sounds safe ;-) thanks, jirka
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y index 545ab7cefc20..4725b14b9db4 100644 --- a/tools/perf/util/parse-events.y +++ b/tools/perf/util/parse-events.y @@ -12,6 +12,7 @@ #include <stdio.h> #include <linux/compiler.h> #include <linux/types.h> +#include <linux/zalloc.h> #include "pmu.h" #include "evsel.h" #include "parse-events.h" @@ -37,6 +38,25 @@ static struct list_head* alloc_list() return list; } +static void free_list_evsel(struct list_head* list_evsel) +{ + struct perf_evsel *pos, *tmp; + + list_for_each_entry_safe(pos, tmp, list_evsel, node) { + list_del_init(&pos->node); + perf_evsel__delete(pos); + } + free(list_evsel); +} + +static void free_term(struct parse_events_term *term) +{ + if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) + free(term->val.str); + zfree(&term->array.ranges); + free(term); +} + static void inc_group_count(struct list_head *list, struct parse_events_state *parse_state) { @@ -66,6 +86,7 @@ static void inc_group_count(struct list_head *list, %type <num> PE_VALUE_SYM_TOOL %type <num> PE_RAW %type <num> PE_TERM +%type <num> value_sym %type <str> PE_NAME %type <str> PE_BPF_OBJECT %type <str> PE_BPF_SOURCE @@ -76,37 +97,43 @@ static void inc_group_count(struct list_head *list, %type <str> PE_EVENT_NAME %type <str> PE_PMU_EVENT_PRE PE_PMU_EVENT_SUF PE_KERNEL_PMU_EVENT %type <str> PE_DRV_CFG_TERM -%type <num> value_sym -%type <head> event_config -%type <head> opt_event_config -%type <head> opt_pmu_config +%destructor { free ($$); } <str> %type <term> event_term -%type <head> event_pmu -%type <head> event_legacy_symbol -%type <head> event_legacy_cache -%type <head> event_legacy_mem -%type <head> event_legacy_tracepoint +%destructor { free_term ($$); } <term> +%type <list_terms> event_config +%type <list_terms> opt_event_config +%type <list_terms> opt_pmu_config +%destructor { parse_events_terms__delete ($$); } <list_terms> +%type <list_evsel> event_pmu +%type <list_evsel> event_legacy_symbol +%type <list_evsel> event_legacy_cache +%type <list_evsel> event_legacy_mem +%type <list_evsel> event_legacy_tracepoint +%type <list_evsel> event_legacy_numeric +%type <list_evsel> event_legacy_raw +%type <list_evsel> event_bpf_file +%type <list_evsel> event_def +%type <list_evsel> event_mod +%type <list_evsel> event_name +%type <list_evsel> event +%type <list_evsel> events +%type <list_evsel> group_def +%type <list_evsel> group +%type <list_evsel> groups +%destructor { free_list_evsel ($$); } <list_evsel> %type <tracepoint_name> tracepoint_name -%type <head> event_legacy_numeric -%type <head> event_legacy_raw -%type <head> event_bpf_file -%type <head> event_def -%type <head> event_mod -%type <head> event_name -%type <head> event -%type <head> events -%type <head> group_def -%type <head> group -%type <head> groups +%destructor { free ($$.sys); free ($$.event); } <tracepoint_name> %type <array> array %type <array> array_term %type <array> array_terms +%destructor { free ($$.ranges); } <array> %union { char *str; u64 num; - struct list_head *head; + struct list_head *list_evsel; + struct list_head *list_terms; struct parse_events_term *term; struct tracepoint_name { char *sys;
If parsing fails then destructors are ran to clean the up the stack. Rename the head union member to make the term and evlist use cases more distinct, this simplifies matching the correct destructor. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/parse-events.y | 69 +++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 21 deletions(-)