| Message ID | 20200120141553.23934-1-cengiz@kernel.wtf |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | tools: perf: add missing unlock to maps__insert error case | expand |
Em Mon, Jan 20, 2020 at 05:15:54PM +0300, Cengiz Can escreveu: > `tools/perf/util/map.c` has a function named `maps__insert` that > acquires a write lock if its in multithread context. > > Even though this lock is released when function successfully completes, > there's a branch that is executed when `maps_by_name == NULL` that > returns from this function without releasing the write lock. > > Added an `up_write` to release the lock when this happens. > > Signed-off-by: Cengiz Can <cengiz@kernel.wtf> > --- > > Hello Arnaldo, > > I'm not exactly sure about the order that we must call up_write here. > > Please tell me if the `__maps__free_maps_by_name` frees the > `rw_semaphore`. If that's the case, should we change the order to unlock and free? No it doesn't free the rw_semaphore, that is in 'struct maps', what is being freed is just something protected by rw_semaphore, maps->maps_by_name, so your patch is right and I'm applying it, thanks. - Arnaldo > Thanks! > > tools/perf/util/map.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c > index fdd5bddb3075..f67960bedebb 100644 > --- a/tools/perf/util/map.c > +++ b/tools/perf/util/map.c > @@ -549,6 +549,7 @@ void maps__insert(struct maps *maps, struct map *map) > > if (maps_by_name == NULL) { > __maps__free_maps_by_name(maps); > + up_write(&maps->lock); > return; > } > > -- > 2.25.0 >
Em Fri, Jan 31, 2020 at 09:38:58AM +0100, Arnaldo Carvalho de Melo escreveu: > Em Mon, Jan 20, 2020 at 05:15:54PM +0300, Cengiz Can escreveu: > > Please tell me if the `__maps__free_maps_by_name` frees the > > `rw_semaphore`. If that's the case, should we change the order to unlock and free? > > No it doesn't free the rw_semaphore, that is in 'struct maps', what is > being freed is just something protected by rw_semaphore, > maps->maps_by_name, so your patch is right and I'm applying it, thanks. BTW, you forgot to add: Fixes: a7c2b572e217 ("perf map_groups: Auto sort maps by name, if needed") Which I did, and next time please CC the perf tools reviewers, as noted in MAINTAINERS, the lines starting with R:. - Arnaldo [acme@quaco perf]$ grep -A21 "PERFORMANCE EVENTS SUBSYSTEM$" MAINTAINERS PERFORMANCE EVENTS SUBSYSTEM M: Peter Zijlstra <peterz@infradead.org> M: Ingo Molnar <mingo@redhat.com> M: Arnaldo Carvalho de Melo <acme@kernel.org> R: Mark Rutland <mark.rutland@arm.com> R: Alexander Shishkin <alexander.shishkin@linux.intel.com> R: Jiri Olsa <jolsa@redhat.com> R: Namhyung Kim <namhyung@kernel.org> L: linux-kernel@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf/core S: Supported F: kernel/events/* F: include/linux/perf_event.h F: include/uapi/linux/perf_event.h F: arch/*/kernel/perf_event*.c F: arch/*/kernel/*/perf_event*.c F: arch/*/kernel/*/*/perf_event*.c F: arch/*/include/asm/perf_event.h F: arch/*/kernel/perf_callchain.c F: arch/*/events/* F: arch/*/events/*/* F: tools/perf/ [acme@quaco perf]$
On January 31, 2020 11:43:46 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > Em Fri, Jan 31, 2020 at 09:38:58AM +0100, Arnaldo Carvalho de Melo escreveu: >> Em Mon, Jan 20, 2020 at 05:15:54PM +0300, Cengiz Can escreveu: >>> Please tell me if the `__maps__free_maps_by_name` frees the >>> `rw_semaphore`. If that's the case, should we change the order to unlock >>> and free? >> >> No it doesn't free the rw_semaphore, that is in 'struct maps', what is >> being freed is just something protected by rw_semaphore, >> maps->maps_by_name, so your patch is right and I'm applying it, thanks. > > BTW, you forgot to add: > > Fixes: a7c2b572e217 ("perf map_groups: Auto sort maps by name, if needed") > > Which I did, and next time please CC the perf tools reviewers, as noted > in MAINTAINERS, the lines starting with R:. Missed that. Thank you for reminding and correction. Cheers > > - Arnaldo > > [acme@quaco perf]$ grep -A21 "PERFORMANCE EVENTS SUBSYSTEM$" MAINTAINERS > PERFORMANCE EVENTS SUBSYSTEM > M: Peter Zijlstra <peterz@infradead.org> > M: Ingo Molnar <mingo@redhat.com> > M: Arnaldo Carvalho de Melo <acme@kernel.org> > R: Mark Rutland <mark.rutland@arm.com> > R: Alexander Shishkin <alexander.shishkin@linux.intel.com> > R: Jiri Olsa <jolsa@redhat.com> > R: Namhyung Kim <namhyung@kernel.org> > L: linux-kernel@vger.kernel.org > T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf/core > S: Supported > F: kernel/events/* > F: include/linux/perf_event.h > F: include/uapi/linux/perf_event.h > F: arch/*/kernel/perf_event*.c > F: arch/*/kernel/*/perf_event*.c > F: arch/*/kernel/*/*/perf_event*.c > F: arch/*/include/asm/perf_event.h > F: arch/*/kernel/perf_callchain.c > F: arch/*/events/* > F: arch/*/events/*/* > F: tools/perf/ > [acme@quaco perf]$
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c index fdd5bddb3075..f67960bedebb 100644 --- a/tools/perf/util/map.c +++ b/tools/perf/util/map.c @@ -549,6 +549,7 @@ void maps__insert(struct maps *maps, struct map *map) if (maps_by_name == NULL) { __maps__free_maps_by_name(maps); + up_write(&maps->lock); return; }
`tools/perf/util/map.c` has a function named `maps__insert` that acquires a write lock if its in multithread context. Even though this lock is released when function successfully completes, there's a branch that is executed when `maps_by_name == NULL` that returns from this function without releasing the write lock. Added an `up_write` to release the lock when this happens. Signed-off-by: Cengiz Can <cengiz@kernel.wtf> --- Hello Arnaldo, I'm not exactly sure about the order that we must call up_write here. Please tell me if the `__maps__free_maps_by_name` frees the `rw_semaphore`. If that's the case, should we change the order to unlock and free? Thanks! tools/perf/util/map.c | 1 + 1 file changed, 1 insertion(+) -- 2.25.0