Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
Hi Namhyung,
On Thu, Sep 27, 2012 at 01:02:05PM +0900, Namhyung Kim wrote:
> Hi Feng,
>
> On Wed, 26 Sep 2012 15:57:07 +0800, Feng Tang wrote:
> > On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote:
> >> Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
> >> > On Tue, 25 Sep 2012 11:11:21 +0900
> >> > Namhyung Kim wrote:
> >> > > Ditto. Plus it might leak previous input_name.
> >> >
> >> > Nice catch, will check the return value of "strdup".
> >> >
> >> > For input_name mem leak, in some cases the input_name can't be called
> >> > with free(), like those got from parse "-i" option. In case the old
> >> > input_name is got from malloc through strdup, I think it's not a big
> >> > issue given that buffer will be freed any way when the application exit.
> >>
> >> I think this is a matter of discipline, leaking is bad, kernel or
> >> userspace, why special case it?
> >>
> >
> > I see, then we need make sure all "input_name" point to a malloced buffer,
> > here is a initial debug patch will only touch the annotate/report/script
> > cmds, pls review and more suggestions are welcomed:
>
> Well, how about adding a flag like "input_name_alloced" and checking it
> before new allocation? This way we can avoid needless strdup when
> runtime switching is not used.
Right, myself have checked that only the runtime switch will malloc a
buffer for the new "input_name", IOW "input_name" in other case can't be
called with free(). So one flag only inside hists.c is ok to cover this,
like the following code.
btw, thanks for your patch about the parse-events()! will try it out.
- Feng
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 6048820..c29335a 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1125,6 +1125,13 @@ static inline void free_popup_options(char **options,
int n)
}
}
+/*
+ * Only runtime switching of perf data file will make "input_name" point
+ * to a malloced buffer. So add "is_input_name_malloced" flag to decide
+ * whether we need to call free() for current "input_name" during the switch.
+ */
+static bool is_input_name_malloced = false;
+
static int switch_data_file(void)
{
char *pwd, *options[256], *abs_path[256], *tmp;
@@ -1186,8 +1193,10 @@ close_file_and_continue:
if (choice < nr_options && choice >= 0) {
tmp = strdup(abs_path[choice]);
if (tmp) {
+ if (is_input_name_malloced)
+ free((void *)input_name);
input_name = tmp;
+ is_input_name_malloced = true;
ret = 0;
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
Hi Feng, On Wed, 26 Sep 2012 15:57:07 +0800, Feng Tang wrote: > On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote: >> Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu: >> > On Tue, 25 Sep 2012 11:11:21 +0900 >> > Namhyung Kim wrote: >> > > Ditto. Plus it might leak previous input_name. >> > >> > Nice catch, will check the return value of "strdup". >> > >> > For input_name mem leak, in some cases the input_name can't be called >> > with free(), like those got from parse "-i" option. In case the old >> > input_name is got from malloc through strdup, I think it's not a big >> > issue given that buffer will be freed any way when the application exit. >> >> I think this is a matter of discipline, leaking is bad, kernel or >> userspace, why special case it? >> > > I see, then we need make sure all "input_name" point to a malloced buffer, > here is a initial debug patch will only touch the annotate/report/script > cmds, pls review and more suggestions are welcomed: Well, how about adding a flag like "input_name_alloced" and checking it before new allocation? This way we can avoid needless strdup when runtime switching is not used. Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
> > On Tue, 25 Sep 2012 11:11:21 +0900
> > Namhyung Kim wrote:
> > > Ditto. Plus it might leak previous input_name.
> >
> > Nice catch, will check the return value of "strdup".
> >
> > For input_name mem leak, in some cases the input_name can't be called
> > with free(), like those got from parse "-i" option. In case the old
> > input_name is got from malloc through strdup, I think it's not a big
> > issue given that buffer will be freed any way when the application exit.
>
> I think this is a matter of discipline, leaking is bad, kernel or
> userspace, why special case it?
>
I see, then we need make sure all "input_name" point to a malloced buffer,
here is a initial debug patch will only touch the annotate/report/script
cmds, pls review and more suggestions are welcomed:
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index b3d60ae..5165660 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -288,6 +288,8 @@ int cmd_annotate(int argc, const char **argv, const char
*prefix __maybe_unused)
argc = parse_options(argc, argv, options, annotate_usage, 0);
+ try_dup_input_name();
+
if (annotate.use_stdio)
use_browser = 0;
else if (annotate.use_tui)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index b4d216b..bec614c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -668,6 +668,8 @@ int cmd_report(int argc, const char **argv, const char
*prefix __maybe_unused)
input_name = "perf.data";
}
+ try_dup_input_name();
+
repeat:
session = perf_session__new(input_name, O_RDONLY,
report.force, false, );
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 30e880c..f13df66 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1477,6 +1477,8 @@ int cmd_script(int argc, const char **argv, const char
*prefix __maybe_unused)
if (!script_name)
setup_pager();
+ try_dup_input_name();
+
session = perf_session__new(input_name, O_RDONLY, 0, false,
_script);
if (session == NULL)
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 2a09de0..d6a097e 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -64,6 +64,20 @@ struct pager_config {
int val;
};
+/*
+ * "input_name" may point to a memory regsion got or not got from
+ * malloc(), this fun will enforce it to point to a malloced region,
+ * this is to avoid memory leak when runtime siwtching perf data file.
+ */
+void try_dup_input_name(void)
+{
+ if (input_name) {
+ input_name = strdup(input_name);
+ if (!input_name)
+ printf("Warning: Fail to duplicate input_name\n");
+ }
+}
+
static int pager_command_config(const char *var, const char *value, void *data)
{
struct pager_config *c = data;
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 42da932..1159213 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -207,6 +207,7 @@ extern bool perf_host, perf_guest;
extern const char perf_version_string[];
void pthread__unblock_sigwinch(void);
+void try_dup_input_name(void);
#include "util/target.h"
Thanks,
Feng
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote:
Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
On Tue, 25 Sep 2012 11:11:21 +0900
Namhyung Kim namhy...@kernel.org wrote:
Ditto. Plus it might leak previous input_name.
Nice catch, will check the return value of strdup.
For input_name mem leak, in some cases the input_name can't be called
with free(), like those got from parse -i option. In case the old
input_name is got from malloc through strdup, I think it's not a big
issue given that buffer will be freed any way when the application exit.
I think this is a matter of discipline, leaking is bad, kernel or
userspace, why special case it?
I see, then we need make sure all input_name point to a malloced buffer,
here is a initial debug patch will only touch the annotate/report/script
cmds, pls review and more suggestions are welcomed:
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index b3d60ae..5165660 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -288,6 +288,8 @@ int cmd_annotate(int argc, const char **argv, const char
*prefix __maybe_unused)
argc = parse_options(argc, argv, options, annotate_usage, 0);
+ try_dup_input_name();
+
if (annotate.use_stdio)
use_browser = 0;
else if (annotate.use_tui)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index b4d216b..bec614c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -668,6 +668,8 @@ int cmd_report(int argc, const char **argv, const char
*prefix __maybe_unused)
input_name = perf.data;
}
+ try_dup_input_name();
+
repeat:
session = perf_session__new(input_name, O_RDONLY,
report.force, false, report.tool);
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 30e880c..f13df66 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1477,6 +1477,8 @@ int cmd_script(int argc, const char **argv, const char
*prefix __maybe_unused)
if (!script_name)
setup_pager();
+ try_dup_input_name();
+
session = perf_session__new(input_name, O_RDONLY, 0, false,
perf_script);
if (session == NULL)
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 2a09de0..d6a097e 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -64,6 +64,20 @@ struct pager_config {
int val;
};
+/*
+ * input_name may point to a memory regsion got or not got from
+ * malloc(), this fun will enforce it to point to a malloced region,
+ * this is to avoid memory leak when runtime siwtching perf data file.
+ */
+void try_dup_input_name(void)
+{
+ if (input_name) {
+ input_name = strdup(input_name);
+ if (!input_name)
+ printf(Warning: Fail to duplicate input_name\n);
+ }
+}
+
static int pager_command_config(const char *var, const char *value, void *data)
{
struct pager_config *c = data;
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 42da932..1159213 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -207,6 +207,7 @@ extern bool perf_host, perf_guest;
extern const char perf_version_string[];
void pthread__unblock_sigwinch(void);
+void try_dup_input_name(void);
#include util/target.h
Thanks,
Feng
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
Hi Feng, On Wed, 26 Sep 2012 15:57:07 +0800, Feng Tang wrote: On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote: Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu: On Tue, 25 Sep 2012 11:11:21 +0900 Namhyung Kim namhy...@kernel.org wrote: Ditto. Plus it might leak previous input_name. Nice catch, will check the return value of strdup. For input_name mem leak, in some cases the input_name can't be called with free(), like those got from parse -i option. In case the old input_name is got from malloc through strdup, I think it's not a big issue given that buffer will be freed any way when the application exit. I think this is a matter of discipline, leaking is bad, kernel or userspace, why special case it? I see, then we need make sure all input_name point to a malloced buffer, here is a initial debug patch will only touch the annotate/report/script cmds, pls review and more suggestions are welcomed: Well, how about adding a flag like input_name_alloced and checking it before new allocation? This way we can avoid needless strdup when runtime switching is not used. Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
Hi Namhyung,
On Thu, Sep 27, 2012 at 01:02:05PM +0900, Namhyung Kim wrote:
Hi Feng,
On Wed, 26 Sep 2012 15:57:07 +0800, Feng Tang wrote:
On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote:
Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
On Tue, 25 Sep 2012 11:11:21 +0900
Namhyung Kim namhy...@kernel.org wrote:
Ditto. Plus it might leak previous input_name.
Nice catch, will check the return value of strdup.
For input_name mem leak, in some cases the input_name can't be called
with free(), like those got from parse -i option. In case the old
input_name is got from malloc through strdup, I think it's not a big
issue given that buffer will be freed any way when the application exit.
I think this is a matter of discipline, leaking is bad, kernel or
userspace, why special case it?
I see, then we need make sure all input_name point to a malloced buffer,
here is a initial debug patch will only touch the annotate/report/script
cmds, pls review and more suggestions are welcomed:
Well, how about adding a flag like input_name_alloced and checking it
before new allocation? This way we can avoid needless strdup when
runtime switching is not used.
Right, myself have checked that only the runtime switch will malloc a
buffer for the new input_name, IOW input_name in other case can't be
called with free(). So one flag only inside hists.c is ok to cover this,
like the following code.
btw, thanks for your patch about the parse-events()! will try it out.
- Feng
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 6048820..c29335a 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1125,6 +1125,13 @@ static inline void free_popup_options(char **options,
int n)
}
}
+/*
+ * Only runtime switching of perf data file will make input_name point
+ * to a malloced buffer. So add is_input_name_malloced flag to decide
+ * whether we need to call free() for current input_name during the switch.
+ */
+static bool is_input_name_malloced = false;
+
static int switch_data_file(void)
{
char *pwd, *options[256], *abs_path[256], *tmp;
@@ -1186,8 +1193,10 @@ close_file_and_continue:
if (choice nr_options choice = 0) {
tmp = strdup(abs_path[choice]);
if (tmp) {
+ if (is_input_name_malloced)
+ free((void *)input_name);
input_name = tmp;
+ is_input_name_malloced = true;
ret = 0;
}
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu: > On Tue, 25 Sep 2012 11:11:21 +0900 > Namhyung Kim wrote: > > Ditto. Plus it might leak previous input_name. > > Nice catch, will check the return value of "strdup". > > For input_name mem leak, in some cases the input_name can't be called > with free(), like those got from parse "-i" option. In case the old > input_name is got from malloc through strdup, I think it's not a big > issue given that buffer will be freed any way when the application exit. I think this is a matter of discipline, leaking is bad, kernel or userspace, why special case it? - Arnaldo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
On Tue, 25 Sep 2012 11:11:21 +0900
Namhyung Kim wrote:
> On Mon, 24 Sep 2012 23:24:10 +0800, Feng Tang wrote:
> [snip]
> > + if (!check_perf_magic(magic)) {
> > + options[nr_options] = strdup(name);
> > + abs_path[nr_options++] = strdup(path);
>
> Need to check return values.
>
>
> > + }
> > + fclose(file);
> > + }
> > + closedir(pwd_dir);
> > +
> > + if (nr_options) {
> > + choice = ui__popup_menu(nr_options, options);
> > + if (choice < nr_options && choice >= 0) {
> > + input_name = strdup(abs_path[choice]);
>
> Ditto. Plus it might leak previous input_name.
Nice catch, will check the return value of "strdup".
For input_name mem leak, in some cases the input_name can't be called
with free(), like those got from parse "-i" option. In case the old
input_name is got from malloc through strdup, I think it's not a big
issue given that buffer will be freed any way when the application exit.
Thanks,
Feng
>
> Thanks,
> Namhyung
>
>
> > + ret = 0;
> > + }
> > + }
> > +
> > + free_popup_options(options, nr_options);
> > + free_popup_options(abs_path, nr_options);
> > + return ret;
> > +}
> > +
> > +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
On Tue, 25 Sep 2012 11:11:21 +0900
Namhyung Kim namhy...@kernel.org wrote:
On Mon, 24 Sep 2012 23:24:10 +0800, Feng Tang wrote:
[snip]
+ if (!check_perf_magic(magic)) {
+ options[nr_options] = strdup(name);
+ abs_path[nr_options++] = strdup(path);
Need to check return values.
+ }
+ fclose(file);
+ }
+ closedir(pwd_dir);
+
+ if (nr_options) {
+ choice = ui__popup_menu(nr_options, options);
+ if (choice nr_options choice = 0) {
+ input_name = strdup(abs_path[choice]);
Ditto. Plus it might leak previous input_name.
Nice catch, will check the return value of strdup.
For input_name mem leak, in some cases the input_name can't be called
with free(), like those got from parse -i option. In case the old
input_name is got from malloc through strdup, I think it's not a big
issue given that buffer will be freed any way when the application exit.
Thanks,
Feng
Thanks,
Namhyung
+ ret = 0;
+ }
+ }
+
+ free_popup_options(options, nr_options);
+ free_popup_options(abs_path, nr_options);
+ return ret;
+}
+
+
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu: On Tue, 25 Sep 2012 11:11:21 +0900 Namhyung Kim namhy...@kernel.org wrote: Ditto. Plus it might leak previous input_name. Nice catch, will check the return value of strdup. For input_name mem leak, in some cases the input_name can't be called with free(), like those got from parse -i option. In case the old input_name is got from malloc through strdup, I think it's not a big issue given that buffer will be freed any way when the application exit. I think this is a matter of discipline, leaking is bad, kernel or userspace, why special case it? - Arnaldo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
On Mon, 24 Sep 2012 23:24:10 +0800, Feng Tang wrote:
[snip]
> + if (!check_perf_magic(magic)) {
> + options[nr_options] = strdup(name);
> + abs_path[nr_options++] = strdup(path);
Need to check return values.
> + }
> + fclose(file);
> + }
> + closedir(pwd_dir);
> +
> + if (nr_options) {
> + choice = ui__popup_menu(nr_options, options);
> + if (choice < nr_options && choice >= 0) {
> + input_name = strdup(abs_path[choice]);
Ditto. Plus it might leak previous input_name.
Thanks,
Namhyung
> + ret = 0;
> + }
> + }
> +
> + free_popup_options(options, nr_options);
> + free_popup_options(abs_path, nr_options);
> + return ret;
> +}
> +
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 8/9] perf hists browser: Add option for runtime switching perf data file
On Mon, 24 Sep 2012 23:24:10 +0800, Feng Tang wrote:
[snip]
+ if (!check_perf_magic(magic)) {
+ options[nr_options] = strdup(name);
+ abs_path[nr_options++] = strdup(path);
Need to check return values.
+ }
+ fclose(file);
+ }
+ closedir(pwd_dir);
+
+ if (nr_options) {
+ choice = ui__popup_menu(nr_options, options);
+ if (choice nr_options choice = 0) {
+ input_name = strdup(abs_path[choice]);
Ditto. Plus it might leak previous input_name.
Thanks,
Namhyung
+ ret = 0;
+ }
+ }
+
+ free_popup_options(options, nr_options);
+ free_popup_options(abs_path, nr_options);
+ return ret;
+}
+
+
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
