Re: [PATCH 04/11] perf test: Fix cpu and thread map leaks in sw_clock_freq test
Em Tue, Mar 02, 2021 at 02:03:05PM +0100, Jiri Olsa escreveu:
> On Tue, Mar 02, 2021 at 10:50:15AM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > > > err = -ENOMEM;
> > > > pr_debug("Not enough memory to create thread/cpu maps\n");
> > > > - goto out_free_maps;
> > > > + goto out_delete_evlist;
> > > > }
> > > >
> > > > perf_evlist__set_maps(>core, cpus, threads);
> > > >
> > > > - cpus= NULL;
> > > > - threads = NULL;
> > >
> > > hum, so IIUC we added these and the other you remove in your patches long
> > > time ago,
> > > because there was no refcounting at that time, right?
> >
> > It seems my original patch just set the maps directly.
> >
> > bc96b361cbf9 perf tests: Add a test case for checking sw clock event
> > frequency
> >
> > And after that Adrian changed it to use the set_maps() helper.
> >
> > c5e6bd2ed3e8 perf tests: Fix software clock events test setting maps
>
> ok, and after that there's this one:
> a55e56637613 perf evlist: Reference count the cpu and thread maps at
> set_maps()
>
> forcing the get calls when storing cpus and threads
>
> for the patchset
>
> Acked-by: Jiri Olsa
Thanks, applied.
- Arnaldo
> thanks,
> jirka
>
> >
> > It seems we already had the refcounting at the moment. And then the libperf
> > renaming happened later.
> >
> > Thanks,
> > Namhyung
> >
>
--
- Arnaldo
Re: [PATCH 04/11] perf test: Fix cpu and thread map leaks in sw_clock_freq test
On Tue, Mar 02, 2021 at 10:50:15AM +0900, Namhyung Kim wrote:
SNIP
> > > err = -ENOMEM;
> > > pr_debug("Not enough memory to create thread/cpu maps\n");
> > > - goto out_free_maps;
> > > + goto out_delete_evlist;
> > > }
> > >
> > > perf_evlist__set_maps(>core, cpus, threads);
> > >
> > > - cpus= NULL;
> > > - threads = NULL;
> >
> > hum, so IIUC we added these and the other you remove in your patches long
> > time ago,
> > because there was no refcounting at that time, right?
>
> It seems my original patch just set the maps directly.
>
> bc96b361cbf9 perf tests: Add a test case for checking sw clock event
> frequency
>
> And after that Adrian changed it to use the set_maps() helper.
>
> c5e6bd2ed3e8 perf tests: Fix software clock events test setting maps
ok, and after that there's this one:
a55e56637613 perf evlist: Reference count the cpu and thread maps at
set_maps()
forcing the get calls when storing cpus and threads
for the patchset
Acked-by: Jiri Olsa
thanks,
jirka
>
> It seems we already had the refcounting at the moment. And then the libperf
> renaming happened later.
>
> Thanks,
> Namhyung
>
Re: [PATCH 04/11] perf test: Fix cpu and thread map leaks in sw_clock_freq test
Hi Jiri,
On Tue, Mar 2, 2021 at 2:24 AM Jiri Olsa wrote:
>
> On Mon, Mar 01, 2021 at 11:04:02PM +0900, Namhyung Kim wrote:
> > The evlist has the maps with its own refcounts so we don't need to set
> > the pointers to NULL. Otherwise following error was reported by Asan.
> >
> > Also change the goto label since it doesn't need to have two.
> >
> > # perf test -v 25
> > 25: Software clock events period values:
> > --- start ---
> > test child forked, pid 149154
> > mmap size 528384B
> > mmap size 528384B
> >
> > =
> > ==149154==ERROR: LeakSanitizer: detected memory leaks
> >
> > Direct leak of 32 byte(s) in 1 object(s) allocated from:
> > #0 0x7fef5cd071f8 in __interceptor_realloc
> > ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:164
> > #1 0x56260d5e8b8e in perf_thread_map__realloc
> > /home/namhyung/project/linux/tools/lib/perf/threadmap.c:23
> > #2 0x56260d3df7a9 in thread_map__new_by_tid util/thread_map.c:63
> > #3 0x56260d2ac6b2 in __test__sw_clock_freq tests/sw-clock.c:65
> > #4 0x56260d26d8fb in run_test tests/builtin-test.c:428
> > #5 0x56260d26d8fb in test_and_print tests/builtin-test.c:458
> > #6 0x56260d26fa53 in __cmd_test tests/builtin-test.c:679
> > #7 0x56260d26fa53 in cmd_test tests/builtin-test.c:825
> > #8 0x56260d2dbb64 in run_builtin
> > /home/namhyung/project/linux/tools/perf/perf.c:313
> > #9 0x56260d165a88 in handle_internal_command
> > /home/namhyung/project/linux/tools/perf/perf.c:365
> > #10 0x56260d165a88 in run_argv
> > /home/namhyung/project/linux/tools/perf/perf.c:409
> > #11 0x56260d165a88 in main
> > /home/namhyung/project/linux/tools/perf/perf.c:539
> > #12 0x7fef5c83cd09 in __libc_start_main ../csu/libc-start.c:308
> >
> > ...
> > test child finished with 1
> > end
> > Software clock events period values : FAILED!
> >
> > Signed-off-by: Namhyung Kim
> > ---
> > tools/perf/tests/sw-clock.c | 12
> > 1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/perf/tests/sw-clock.c b/tools/perf/tests/sw-clock.c
> > index a49c9e23053b..74988846be1d 100644
> > --- a/tools/perf/tests/sw-clock.c
> > +++ b/tools/perf/tests/sw-clock.c
> > @@ -42,8 +42,8 @@ static int __test__sw_clock_freq(enum perf_sw_ids
> > clock_id)
> > .disabled = 1,
> > .freq = 1,
> > };
> > - struct perf_cpu_map *cpus;
> > - struct perf_thread_map *threads;
> > + struct perf_cpu_map *cpus = NULL;
> > + struct perf_thread_map *threads = NULL;
> > struct mmap *md;
> >
> > attr.sample_freq = 500;
> > @@ -66,14 +66,11 @@ static int __test__sw_clock_freq(enum perf_sw_ids
> > clock_id)
> > if (!cpus || !threads) {
> > err = -ENOMEM;
> > pr_debug("Not enough memory to create thread/cpu maps\n");
> > - goto out_free_maps;
> > + goto out_delete_evlist;
> > }
> >
> > perf_evlist__set_maps(>core, cpus, threads);
> >
> > - cpus= NULL;
> > - threads = NULL;
>
> hum, so IIUC we added these and the other you remove in your patches long
> time ago,
> because there was no refcounting at that time, right?
It seems my original patch just set the maps directly.
bc96b361cbf9 perf tests: Add a test case for checking sw clock event frequency
And after that Adrian changed it to use the set_maps() helper.
c5e6bd2ed3e8 perf tests: Fix software clock events test setting maps
It seems we already had the refcounting at the moment. And then the libperf
renaming happened later.
Thanks,
Namhyung
Re: [PATCH 04/11] perf test: Fix cpu and thread map leaks in sw_clock_freq test
On Mon, Mar 01, 2021 at 11:04:02PM +0900, Namhyung Kim wrote:
> The evlist has the maps with its own refcounts so we don't need to set
> the pointers to NULL. Otherwise following error was reported by Asan.
>
> Also change the goto label since it doesn't need to have two.
>
> # perf test -v 25
> 25: Software clock events period values:
> --- start ---
> test child forked, pid 149154
> mmap size 528384B
> mmap size 528384B
>
> =
> ==149154==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 32 byte(s) in 1 object(s) allocated from:
> #0 0x7fef5cd071f8 in __interceptor_realloc
> ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:164
> #1 0x56260d5e8b8e in perf_thread_map__realloc
> /home/namhyung/project/linux/tools/lib/perf/threadmap.c:23
> #2 0x56260d3df7a9 in thread_map__new_by_tid util/thread_map.c:63
> #3 0x56260d2ac6b2 in __test__sw_clock_freq tests/sw-clock.c:65
> #4 0x56260d26d8fb in run_test tests/builtin-test.c:428
> #5 0x56260d26d8fb in test_and_print tests/builtin-test.c:458
> #6 0x56260d26fa53 in __cmd_test tests/builtin-test.c:679
> #7 0x56260d26fa53 in cmd_test tests/builtin-test.c:825
> #8 0x56260d2dbb64 in run_builtin
> /home/namhyung/project/linux/tools/perf/perf.c:313
> #9 0x56260d165a88 in handle_internal_command
> /home/namhyung/project/linux/tools/perf/perf.c:365
> #10 0x56260d165a88 in run_argv
> /home/namhyung/project/linux/tools/perf/perf.c:409
> #11 0x56260d165a88 in main
> /home/namhyung/project/linux/tools/perf/perf.c:539
> #12 0x7fef5c83cd09 in __libc_start_main ../csu/libc-start.c:308
>
> ...
> test child finished with 1
> end
> Software clock events period values : FAILED!
>
> Signed-off-by: Namhyung Kim
> ---
> tools/perf/tests/sw-clock.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/tests/sw-clock.c b/tools/perf/tests/sw-clock.c
> index a49c9e23053b..74988846be1d 100644
> --- a/tools/perf/tests/sw-clock.c
> +++ b/tools/perf/tests/sw-clock.c
> @@ -42,8 +42,8 @@ static int __test__sw_clock_freq(enum perf_sw_ids clock_id)
> .disabled = 1,
> .freq = 1,
> };
> - struct perf_cpu_map *cpus;
> - struct perf_thread_map *threads;
> + struct perf_cpu_map *cpus = NULL;
> + struct perf_thread_map *threads = NULL;
> struct mmap *md;
>
> attr.sample_freq = 500;
> @@ -66,14 +66,11 @@ static int __test__sw_clock_freq(enum perf_sw_ids
> clock_id)
> if (!cpus || !threads) {
> err = -ENOMEM;
> pr_debug("Not enough memory to create thread/cpu maps\n");
> - goto out_free_maps;
> + goto out_delete_evlist;
> }
>
> perf_evlist__set_maps(>core, cpus, threads);
>
> - cpus= NULL;
> - threads = NULL;
hum, so IIUC we added these and the other you remove in your patches long time
ago,
because there was no refcounting at that time, right?
jirka
> -
> if (evlist__open(evlist)) {
> const char *knob =
> "/proc/sys/kernel/perf_event_max_sample_rate";
>
> @@ -129,10 +126,9 @@ static int __test__sw_clock_freq(enum perf_sw_ids
> clock_id)
> err = -1;
> }
>
> -out_free_maps:
> +out_delete_evlist:
> perf_cpu_map__put(cpus);
> perf_thread_map__put(threads);
> -out_delete_evlist:
> evlist__delete(evlist);
> return err;
> }
> --
> 2.30.1.766.gb4fecdf3b7-goog
>
[PATCH 04/11] perf test: Fix cpu and thread map leaks in sw_clock_freq test
The evlist has the maps with its own refcounts so we don't need to set
the pointers to NULL. Otherwise following error was reported by Asan.
Also change the goto label since it doesn't need to have two.
# perf test -v 25
25: Software clock events period values:
--- start ---
test child forked, pid 149154
mmap size 528384B
mmap size 528384B
=
==149154==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x7fef5cd071f8 in __interceptor_realloc
../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:164
#1 0x56260d5e8b8e in perf_thread_map__realloc
/home/namhyung/project/linux/tools/lib/perf/threadmap.c:23
#2 0x56260d3df7a9 in thread_map__new_by_tid util/thread_map.c:63
#3 0x56260d2ac6b2 in __test__sw_clock_freq tests/sw-clock.c:65
#4 0x56260d26d8fb in run_test tests/builtin-test.c:428
#5 0x56260d26d8fb in test_and_print tests/builtin-test.c:458
#6 0x56260d26fa53 in __cmd_test tests/builtin-test.c:679
#7 0x56260d26fa53 in cmd_test tests/builtin-test.c:825
#8 0x56260d2dbb64 in run_builtin
/home/namhyung/project/linux/tools/perf/perf.c:313
#9 0x56260d165a88 in handle_internal_command
/home/namhyung/project/linux/tools/perf/perf.c:365
#10 0x56260d165a88 in run_argv
/home/namhyung/project/linux/tools/perf/perf.c:409
#11 0x56260d165a88 in main
/home/namhyung/project/linux/tools/perf/perf.c:539
#12 0x7fef5c83cd09 in __libc_start_main ../csu/libc-start.c:308
...
test child finished with 1
end
Software clock events period values : FAILED!
Signed-off-by: Namhyung Kim
---
tools/perf/tests/sw-clock.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/tools/perf/tests/sw-clock.c b/tools/perf/tests/sw-clock.c
index a49c9e23053b..74988846be1d 100644
--- a/tools/perf/tests/sw-clock.c
+++ b/tools/perf/tests/sw-clock.c
@@ -42,8 +42,8 @@ static int __test__sw_clock_freq(enum perf_sw_ids clock_id)
.disabled = 1,
.freq = 1,
};
- struct perf_cpu_map *cpus;
- struct perf_thread_map *threads;
+ struct perf_cpu_map *cpus = NULL;
+ struct perf_thread_map *threads = NULL;
struct mmap *md;
attr.sample_freq = 500;
@@ -66,14 +66,11 @@ static int __test__sw_clock_freq(enum perf_sw_ids clock_id)
if (!cpus || !threads) {
err = -ENOMEM;
pr_debug("Not enough memory to create thread/cpu maps\n");
- goto out_free_maps;
+ goto out_delete_evlist;
}
perf_evlist__set_maps(>core, cpus, threads);
- cpus= NULL;
- threads = NULL;
-
if (evlist__open(evlist)) {
const char *knob =
"/proc/sys/kernel/perf_event_max_sample_rate";
@@ -129,10 +126,9 @@ static int __test__sw_clock_freq(enum perf_sw_ids clock_id)
err = -1;
}
-out_free_maps:
+out_delete_evlist:
perf_cpu_map__put(cpus);
perf_thread_map__put(threads);
-out_delete_evlist:
evlist__delete(evlist);
return err;
}
--
2.30.1.766.gb4fecdf3b7-goog
