From: Arnaldo Carvalho de Melo <[email protected]>
So that we can reduce the noise on valgrind when looking for memory
leaks.
Cc: Frederic Weisbecker <[email protected]>
Cc: Mike Galbraith <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Stephane Eranian <[email protected]>
LKML-Reference: <new-submission>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-record.c | 32 ++++++++++++++++++++++++++------
1 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index b938796..5ae0d93 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -439,6 +439,7 @@ static void atexit_header(void)
process_buildids();
perf_header__write(&session->header, output, true);
+ perf_session__delete(session);
}
}
@@ -558,12 +559,15 @@ static int __cmd_record(int argc, const char **argv)
if (!file_new) {
err = perf_header__read(session, output);
if (err < 0)
- return err;
+ goto out_delete_session;
}
if (have_tracepoints(attrs, nr_counters))
perf_header__set_feat(&session->header, HEADER_TRACE_INFO);
+ /*
+ * perf_session__delete(session) will be called at atexit_header()
+ */
atexit(atexit_header);
if (forks) {
@@ -768,6 +772,10 @@ static int __cmd_record(int argc, const char **argv)
bytes_written / 24);
return 0;
+
+out_delete_session:
+ perf_session__delete(session);
+ return err;
}
static const char * const record_usage[] = {
@@ -824,7 +832,7 @@ static const struct option options[] = {
int cmd_record(int argc, const char **argv, const char *prefix __used)
{
- int i,j;
+ int i, j, err = -ENOMEM;
argc = parse_options(argc, argv, options, record_usage,
PARSE_OPT_STOP_AT_NON_OPTION);
@@ -873,13 +881,13 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
for (j = 0; j < MAX_COUNTERS; j++) {
fd[i][j] = malloc(sizeof(int)*thread_num);
if (!fd[i][j])
- return -ENOMEM;
+ goto out_free_fd;
}
}
event_array = malloc(
sizeof(struct pollfd)*MAX_NR_CPUS*MAX_COUNTERS*thread_num);
if (!event_array)
- return -ENOMEM;
+ goto out_free_fd;
if (user_interval != ULLONG_MAX)
default_interval = user_interval;
@@ -895,8 +903,20 @@ int cmd_record(int argc, const char **argv, const char *prefix __used)
default_interval = freq;
} else {
fprintf(stderr, "frequency and count are zero, aborting\n");
- exit(EXIT_FAILURE);
+ err = -EINVAL;
+ goto out_free_event_array;
}
- return __cmd_record(argc, argv);
+ err = __cmd_record(argc, argv);
+
+out_free_event_array:
+ free(event_array);
+out_free_fd:
+ for (i = 0; i < MAX_NR_CPUS; i++) {
+ for (j = 0; j < MAX_COUNTERS; j++)
+ free(fd[i][j]);
+ }
+ free(all_tids);
+ all_tids = NULL;
+ return err;
}
--
1.6.2.5
On Sun, Aug 01, 2010 at 10:08:46PM -0300, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <[email protected]>
>
> So that we can reduce the noise on valgrind when looking for memory
> leaks.
Really? That's rather crappy of valgrind. exit is well defined to
release resources and that's often a more efficient way to do it
It finds and batches things a lot better, eg. it can avoid all
TLB flushing of freeing memory that munmap requires.
* Nick Piggin <[email protected]> wrote:
> On Sun, Aug 01, 2010 at 10:08:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <[email protected]>
> >
> > So that we can reduce the noise on valgrind when looking for memory
> > leaks.
>
> Really? That's rather crappy of valgrind. exit is well defined to release
> resources and that's often a more efficient way to do it It finds and
> batches things a lot better, eg. it can avoid all TLB flushing of freeing
> memory that munmap requires.
That's certainly true but there's no valgrind crappiness here: valgrind simply
can do a better job of finding leaks if there's a well defined "all resources
the app still knows about are freed now" point.
The _kernel_ obviously can release all resources. The distinction is between
'resources known to the app' and 'all resources'. That set contains the
leaking resources.
Thanks,
Ingo
On Mon, Aug 02, 2010 at 09:54:22AM +0200, Ingo Molnar wrote:
>
> * Nick Piggin <[email protected]> wrote:
>
> > On Sun, Aug 01, 2010 at 10:08:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > > From: Arnaldo Carvalho de Melo <[email protected]>
> > >
> > > So that we can reduce the noise on valgrind when looking for memory
> > > leaks.
> >
> > Really? That's rather crappy of valgrind. exit is well defined to release
> > resources and that's often a more efficient way to do it It finds and
> > batches things a lot better, eg. it can avoid all TLB flushing of freeing
> > memory that munmap requires.
>
> That's certainly true but there's no valgrind crappiness here: valgrind simply
> can do a better job of finding leaks if there's a well defined "all resources
> the app still knows about are freed now" point.
"noise" sounds like false positives though. Certainly if this is
instead allows valgrind to run in a particular mode that assumes
no application resources consumed at exit(2) time, I wouldn't
call it crappy :)
But you could equally sprinkle in other valgrind specific annotations
or semantics at various points in the code to improve its coverage,
no?
On Mon, Aug 2, 2010 at 4:08 AM, Arnaldo Carvalho de Melo
<[email protected]> wrote:
> From: Arnaldo Carvalho de Melo <[email protected]>
>
> So that we can reduce the noise on valgrind when looking for memory
> leaks.
>
> Cc: Frederic Weisbecker <[email protected]>
> Cc: Mike Galbraith <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Stephane Eranian <[email protected]>
> LKML-Reference: <new-submission>
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
Yes, please.
Acked-by: Pekka Enberg <[email protected]>
* Nick Piggin <[email protected]> wrote:
> On Mon, Aug 02, 2010 at 09:54:22AM +0200, Ingo Molnar wrote:
> >
> > * Nick Piggin <[email protected]> wrote:
> >
> > > On Sun, Aug 01, 2010 at 10:08:46PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > From: Arnaldo Carvalho de Melo <[email protected]>
> > > >
> > > > So that we can reduce the noise on valgrind when looking for memory
> > > > leaks.
> > >
> > > Really? That's rather crappy of valgrind. exit is well defined to release
> > > resources and that's often a more efficient way to do it It finds and
> > > batches things a lot better, eg. it can avoid all TLB flushing of freeing
> > > memory that munmap requires.
> >
> > That's certainly true but there's no valgrind crappiness here: valgrind simply
> > can do a better job of finding leaks if there's a well defined "all resources
> > the app still knows about are freed now" point.
>
> "noise" sounds like false positives though. [...]
Every predictive bug detection scheme is open to the potential of false
positives. I've yet to see a complex one that is 100% false positive free.
> [...] Certainly if this is instead allows valgrind to run in a particular
> mode that assumes no application resources consumed at exit(2) time, I
> wouldn't call it crappy :)
Most apps free their stuff before they exit - i do it in all my own C apps.
That is generally useful: for example it makes it easier to thread a program
later on - when exit() becomes pthread_exit() and a silent leak turns into a
real leak.
Hence Valgrind checking for exit() by default looks useful to me.
> But you could equally sprinkle in other valgrind specific annotations or
> semantics at various points in the code to improve its coverage, no?
Yeah, and exit() sounds like a pretty convenient point, right? That's the
point where all resources are inactive hence a scan for leaks is expected to
be the most efficient in finding real leaks.
Thanks,
Ingo
On Mon, Aug 02, 2010 at 11:17:14AM +0200, Ingo Molnar wrote:
>
> * Nick Piggin <[email protected]> wrote:
>
> > On Mon, Aug 02, 2010 at 09:54:22AM +0200, Ingo Molnar wrote:
> > >
> > > * Nick Piggin <[email protected]> wrote:
> > >
> > > That's certainly true but there's no valgrind crappiness here: valgrind simply
> > > can do a better job of finding leaks if there's a well defined "all resources
> > > the app still knows about are freed now" point.
> >
> > "noise" sounds like false positives though. [...]
>
> Every predictive bug detection scheme is open to the potential of false
> positives. I've yet to see a complex one that is 100% false positive free.
So long as false positive noise can be easily avoided in running
of the automated testing -- ie. not worked around in the software --
then this is totally fine of course.
> > [...] Certainly if this is instead allows valgrind to run in a particular
> > mode that assumes no application resources consumed at exit(2) time, I
> > wouldn't call it crappy :)
>
> Most apps free their stuff before they exit - i do it in all my own C apps.
>
> That is generally useful: for example it makes it easier to thread a program
> later on - when exit() becomes pthread_exit() and a silent leak turns into a
> real leak.
>
> Hence Valgrind checking for exit() by default looks useful to me.
Sure, most of the time I would too (in fact, all the time seeing as
I don't write non-trivial user programs). But when you're doing last
pass optimisations, it would be very reasonable to avoid things like
teardown of complex data structures.
> > But you could equally sprinkle in other valgrind specific annotations or
> > semantics at various points in the code to improve its coverage, no?
>
> Yeah, and exit() sounds like a pretty convenient point, right? That's the
> point where all resources are inactive hence a scan for leaks is expected to
> be the most efficient in finding real leaks.
Em Mon, Aug 02, 2010 at 07:59:47PM +1000, Nick Piggin escreveu:
> On Mon, Aug 02, 2010 at 11:17:14AM +0200, Ingo Molnar wrote:
> > Most apps free their stuff before they exit - i do it in all my own C apps.
> >
> > That is generally useful: for example it makes it easier to thread a program
> > later on - when exit() becomes pthread_exit() and a silent leak turns into a
> > real leak.
Right, part of my goal was exactly the potential librarization of
nowaday main routines of builtin commands.
> > Hence Valgrind checking for exit() by default looks useful to me.
>
> Sure, most of the time I would too (in fact, all the time seeing as
> I don't write non-trivial user programs). But when you're doing last
> pass optimisations, it would be very reasonable to avoid things like
> teardown of complex data structures.
Yeah, I thought about having some test to only do the on-exit
destruction of lots of rbtrees if in debugging mode, and I may end up
doing it at some point.
But on another angle, right now I think we really need to more
aggressively delete stuff (like maps that had no hits on
PERF_RECORD_EXIT'ed threads and thus have no reference on any hist_entry
instance) to support huge perf.data file buildid exit processing. So
part of the goal was that: no leaks left behind policy.
That is, till the day when we stash the 20 byte buildid in the per
binary image loaded kernel data structure so that we can have it in the
PERF_RECORD_MMAP and avoid all this processing at exit, and instead do
it taking advantage of the existing noise at loading time. 8-)
- Arnaldo