[v3,09/18] perf ui: Update use of pthread mutex

Message ID	20220824153901.488576-10-irogers@google.com (mailing list archive)
State	Superseded
Delegated to:	BPF
Headers	show Return-Path: <bpf-owner@kernel.org> Date: Wed, 24 Aug 2022 08:38:52 -0700 In-Reply-To: <20220824153901.488576-1-irogers@google.com> Message-Id: <20220824153901.488576-10-irogers@google.com> Mime-Version: 1.0 References: <20220824153901.488576-1-irogers@google.com> Subject: [PATCH v3 09/18] perf ui: Update use of pthread mutex From: Ian Rogers <irogers@google.com> To: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Darren Hart <dvhart@infradead.org>, Davidlohr Bueso <dave@stgolabs.net>, " =?utf-8?q?Andr=C3=A9_Almeida?= " <andrealmeid@igalia.com>, Nathan Chancellor <nathan@kernel.org>, Nick Desaulniers <ndesaulniers@google.com>, Tom Rix <trix@redhat.com>, Weiguo Li <liwg06@foxmail.com>, Athira Rajeev <atrajeev@linux.vnet.ibm.com>, Thomas Richter <tmricht@linux.ibm.com>, Ravi Bangoria <ravi.bangoria@amd.com>, Dario Petrillo <dario.pk1@gmail.com>, Hewenliang <hewenliang4@huawei.com>, yaowenbin <yaowenbin1@huawei.com>, Wenyu Liu <liuwenyu7@huawei.com>, Song Liu <songliubraving@fb.com>, Andrii Nakryiko <andrii@kernel.org>, Dave Marchevsky <davemarchevsky@fb.com>, Leo Yan <leo.yan@linaro.org>, Kim Phillips <kim.phillips@amd.com>, Pavithra Gurushankar <gpavithrasha@gmail.com>, Alexandre Truong <alexandre.truong@arm.com>, Quentin Monnet <quentin@isovalent.com>, William Cohen <wcohen@redhat.com>, Andres Freund <andres@anarazel.de>, Adrian Hunter <adrian.hunter@intel.com>, " =?utf-8?q?Martin_Li=C5=A1ka?= " <mliska@suse.cz>, Colin Ian King <colin.king@intel.com>, James Clark <james.clark@arm.com>, Fangrui Song <maskray@google.com>, Stephane Eranian <eranian@google.com>, Kajol Jain <kjain@linux.ibm.com>, Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>, Riccardo Mancini <rickyman7@gmail.com>, Andi Kleen <ak@linux.intel.com>, Masami Hiramatsu <mhiramat@kernel.org>, Zechuan Chen <chenzechuan1@huawei.com>, Jason Wang <wangborong@cdjrlc.com>, Christophe JAILLET <christophe.jaillet@wanadoo.fr>, Remi Bernon <rbernon@codeweavers.com>, linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, bpf@vger.kernel.org, llvm@lists.linux.dev Cc: Ian Rogers <irogers@google.com> Content-Type: text/plain; charset="UTF-8" Precedence: bulk
Series	Mutex wrapper, locking and memory leak fixes \| expand [v3,00/18] Mutex wrapper, locking and memory leak fixes [v3,01/18] perf mutex: Wrapped usage of mutex and cond [v3,02/18] perf bench: Update use of pthread mutex/cond [v3,03/18] perf tests: Avoid pthread.h inclusion [v3,04/18] perf hist: Update use of pthread mutex [v3,05/18] perf bpf: Remove unused pthread.h include [v3,06/18] perf lock: Remove unused pthread.h include [v3,07/18] perf record: Update use of pthread mutex [v3,08/18] perf sched: Update use of pthread mutex [v3,09/18] perf ui: Update use of pthread mutex [v3,10/18] perf mmap: Remove unnecessary pthread.h include [v3,11/18] perf dso: Update use of pthread mutex [v3,12/18] perf annotate: Update use of pthread mutex [v3,13/18] perf top: Update use of pthread mutex [v3,14/18] perf dso: Hold lock when accessing nsinfo [v3,15/18] perf mutex: Add thread safety annotations [v3,16/18] perf sched: Fixes for thread safety analysis [v3,17/18] perf top: Fixes for thread safety analysis [v3,18/18] perf build: Enable -Wthread-safety with clang

Context	Check	Description
bpf/vmtest-bpf-PR	fail	merge-conflict
netdev/tree_selection	success	Not a local patch

Ian Rogers Aug. 24, 2022, 3:38 p.m. UTC

Switch to the use of mutex wrappers that provide better error checking.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browser.c           | 20 ++++++++++----------
 tools/perf/ui/browsers/annotate.c |  2 +-
 tools/perf/ui/setup.c             |  5 +++--
 tools/perf/ui/tui/helpline.c      |  5 ++---
 tools/perf/ui/tui/progress.c      |  8 ++++----
 tools/perf/ui/tui/setup.c         |  8 ++++----
 tools/perf/ui/tui/util.c          | 18 +++++++++---------
 tools/perf/ui/ui.h                |  4 ++--
 8 files changed, 35 insertions(+), 35 deletions(-)

Adrian Hunter Aug. 26, 2022, 10:11 a.m. UTC | #1

On 24/08/22 18:38, Ian Rogers wrote:
> Switch to the use of mutex wrappers that provide better error checking.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/ui/browser.c           | 20 ++++++++++----------
>  tools/perf/ui/browsers/annotate.c |  2 +-

Other changes to tools/perf/ui/browsers/annotate.c are in patch 12

>  tools/perf/ui/setup.c             |  5 +++--
>  tools/perf/ui/tui/helpline.c      |  5 ++---
>  tools/perf/ui/tui/progress.c      |  8 ++++----
>  tools/perf/ui/tui/setup.c         |  8 ++++----
>  tools/perf/ui/tui/util.c          | 18 +++++++++---------
>  tools/perf/ui/ui.h                |  4 ++--
>  8 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> index fa5bd5c20e96..78fb01d6ad63 100644
> --- a/tools/perf/ui/browser.c
> +++ b/tools/perf/ui/browser.c
> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
>  
>  void ui_browser__show_title(struct ui_browser *browser, const char *title)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui_browser__show_title(browser, title);
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  int ui_browser__show(struct ui_browser *browser, const char *title,
> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>  
>  	browser->refresh_dimensions(browser);
>  
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui_browser__show_title(browser, title);
>  
>  	browser->title = title;
> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>  	va_end(ap);
>  	if (err > 0)
>  		ui_helpline__push(browser->helpline);
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  	return err ? 0 : -1;
>  }
>  
>  void ui_browser__hide(struct ui_browser *browser)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	ui_helpline__pop();
>  	zfree(&browser->helpline);
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  static void ui_browser__scrollbar_set(struct ui_browser *browser)
> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
>  
>  int ui_browser__refresh(struct ui_browser *browser)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui_browser__refresh(browser);
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  
>  	return 0;
>  }
> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
>  	while (1) {
>  		off_t offset;
>  
> -		pthread_mutex_lock(&ui__lock);
> +		mutex_lock(&ui__lock);
>  		err = __ui_browser__refresh(browser);
>  		SLsmg_refresh();
> -		pthread_mutex_unlock(&ui__lock);
> +		mutex_unlock(&ui__lock);
>  		if (err < 0)
>  			break;
>  
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 44ba900828f6..b8747e8dd9ea 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -8,11 +8,11 @@
>  #include "../../util/hist.h"
>  #include "../../util/sort.h"
>  #include "../../util/map.h"
> +#include "../../util/mutex.h"
>  #include "../../util/symbol.h"
>  #include "../../util/evsel.h"
>  #include "../../util/evlist.h"
>  #include <inttypes.h>
> -#include <pthread.h>
>  #include <linux/kernel.h>
>  #include <linux/string.h>
>  #include <linux/zalloc.h>
> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> index 700335cde618..25ded88801a3 100644
> --- a/tools/perf/ui/setup.c
> +++ b/tools/perf/ui/setup.c
> @@ -1,5 +1,4 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include <pthread.h>
>  #include <dlfcn.h>
>  #include <unistd.h>
>  
> @@ -8,7 +7,7 @@
>  #include "../util/hist.h"
>  #include "ui.h"
>  
> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> +struct mutex ui__lock;
>  void *perf_gtk_handle;
>  int use_browser = -1;
>  
> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>  
>  void setup_browser(bool fallback_to_pager)
>  {
> +	mutex_init(&ui__lock);
>  	if (use_browser < 2 && (!isatty(1) || dump_trace))
>  		use_browser = 0;
>  
> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
>  	default:
>  		break;
>  	}
> +	mutex_destroy(&ui__lock);
>  }
> diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> index 298d6af82fdd..db4952f5990b 100644
> --- a/tools/perf/ui/tui/helpline.c
> +++ b/tools/perf/ui/tui/helpline.c
> @@ -2,7 +2,6 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#include <pthread.h>
>  #include <linux/kernel.h>
>  #include <linux/string.h>
>  
> @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
>  	int ret;
>  	static int backlog;
>  
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	ret = vscnprintf(ui_helpline__last_msg + backlog,
>  			sizeof(ui_helpline__last_msg) - backlog, format, ap);
>  	backlog += ret;
> @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
>  		SLsmg_refresh();
>  		backlog = 0;
>  	}
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  
>  	return ret;
>  }
> diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> index 3d74af5a7ece..71b6c8d9474f 100644
> --- a/tools/perf/ui/tui/progress.c
> +++ b/tools/perf/ui/tui/progress.c
> @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
>  	}
>  
>  	ui__refresh_dimensions(false);
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	y = SLtt_Screen_Rows / 2 - 2;
>  	SLsmg_set_color(0);
>  	SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
>  	bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
>  	SLsmg_fill_region(y, 1, 1, bar, ' ');
>  	SLsmg_refresh();
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  static void tui_progress__finish(void)
> @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
>  		return;
>  
>  	ui__refresh_dimensions(false);
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	y = SLtt_Screen_Rows / 2 - 2;
>  	SLsmg_set_color(0);
>  	SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
>  	SLsmg_refresh();
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  static struct ui_progress_ops tui_progress__ops = {
> diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> index b1be59b4e2a4..a3b8c397c24d 100644
> --- a/tools/perf/ui/tui/setup.c
> +++ b/tools/perf/ui/tui/setup.c
> @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
>  {
>  	if (force || ui__need_resize) {
>  		ui__need_resize = 0;
> -		pthread_mutex_lock(&ui__lock);
> +		mutex_lock(&ui__lock);
>  		SLtt_get_screen_size();
>  		SLsmg_reinit_smg();
> -		pthread_mutex_unlock(&ui__lock);
> +		mutex_unlock(&ui__lock);
>  	}
>  }
>  
> @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
>  				    "Press any key...", 0);
>  
>  	SLtt_set_cursor_visibility(1);
> -	if (!pthread_mutex_trylock(&ui__lock)) {
> +	if (mutex_trylock(&ui__lock)) {
>  		SLsmg_refresh();
>  		SLsmg_reset_smg();
> -		pthread_mutex_unlock(&ui__lock);
> +		mutex_unlock(&ui__lock);
>  	}
>  	SLang_reset_tty();
>  	perf_error__unregister(&perf_tui_eops);
> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> index 0f562e2cb1e8..3c5174854ac8 100644
> --- a/tools/perf/ui/tui/util.c
> +++ b/tools/perf/ui/tui/util.c
> @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>  		t = sep + 1;
>  	}
>  
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  
>  	max_len += 2;
>  	nr_lines += 8;
> @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>  	SLsmg_write_nstring((char *)exit_msg, max_len);
>  	SLsmg_refresh();
>  
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  
>  	x += 2;
>  	len = 0;
>  	key = ui__getch(delay_secs);
>  	while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> -		pthread_mutex_lock(&ui__lock);
> +		mutex_lock(&ui__lock);
>  
>  		if (key == K_BKSPC) {
>  			if (len == 0) {
> -				pthread_mutex_unlock(&ui__lock);
> +				mutex_unlock(&ui__lock);
>  				goto next_key;
>  			}
>  			SLsmg_gotorc(y, x + --len);
> @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>  		}
>  		SLsmg_refresh();
>  
> -		pthread_mutex_unlock(&ui__lock);
> +		mutex_unlock(&ui__lock);
>  
>  		/* XXX more graceful overflow handling needed */
>  		if (len == sizeof(buf) - 1) {
> @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
>  
>  void ui__info_window(const char *title, const char *text)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui__info_window(title, text, NULL);
>  	SLsmg_refresh();
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  int ui__question_window(const char *title, const char *text,
>  			const char *exit_msg, int delay_secs)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui__info_window(title, text, exit_msg);
>  	SLsmg_refresh();
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  	return ui__getch(delay_secs);
>  }
>  
> diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> --- a/tools/perf/ui/ui.h
> +++ b/tools/perf/ui/ui.h
> @@ -2,11 +2,11 @@
>  #ifndef _PERF_UI_H_
>  #define _PERF_UI_H_ 1
>  
> -#include <pthread.h>
> +#include "../util/mutex.h"
>  #include <stdbool.h>
>  #include <linux/compiler.h>
>  
> -extern pthread_mutex_t ui__lock;
> +extern struct mutex ui__lock;
>  extern void *perf_gtk_handle;
>  
>  extern int use_browser;

Adrian Hunter Aug. 26, 2022, 10:24 a.m. UTC | #2

On 24/08/22 18:38, Ian Rogers wrote:
> Switch to the use of mutex wrappers that provide better error checking.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/ui/browser.c           | 20 ++++++++++----------
>  tools/perf/ui/browsers/annotate.c |  2 +-
>  tools/perf/ui/setup.c             |  5 +++--
>  tools/perf/ui/tui/helpline.c      |  5 ++---
>  tools/perf/ui/tui/progress.c      |  8 ++++----
>  tools/perf/ui/tui/setup.c         |  8 ++++----
>  tools/perf/ui/tui/util.c          | 18 +++++++++---------
>  tools/perf/ui/ui.h                |  4 ++--
>  8 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> index fa5bd5c20e96..78fb01d6ad63 100644
> --- a/tools/perf/ui/browser.c
> +++ b/tools/perf/ui/browser.c
> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
>  
>  void ui_browser__show_title(struct ui_browser *browser, const char *title)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui_browser__show_title(browser, title);
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  int ui_browser__show(struct ui_browser *browser, const char *title,
> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>  
>  	browser->refresh_dimensions(browser);
>  
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui_browser__show_title(browser, title);
>  
>  	browser->title = title;
> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>  	va_end(ap);
>  	if (err > 0)
>  		ui_helpline__push(browser->helpline);
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  	return err ? 0 : -1;
>  }
>  
>  void ui_browser__hide(struct ui_browser *browser)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	ui_helpline__pop();
>  	zfree(&browser->helpline);
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  static void ui_browser__scrollbar_set(struct ui_browser *browser)
> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
>  
>  int ui_browser__refresh(struct ui_browser *browser)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui_browser__refresh(browser);
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  
>  	return 0;
>  }
> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
>  	while (1) {
>  		off_t offset;
>  
> -		pthread_mutex_lock(&ui__lock);
> +		mutex_lock(&ui__lock);
>  		err = __ui_browser__refresh(browser);
>  		SLsmg_refresh();
> -		pthread_mutex_unlock(&ui__lock);
> +		mutex_unlock(&ui__lock);
>  		if (err < 0)
>  			break;
>  
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 44ba900828f6..b8747e8dd9ea 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -8,11 +8,11 @@
>  #include "../../util/hist.h"
>  #include "../../util/sort.h"
>  #include "../../util/map.h"
> +#include "../../util/mutex.h"
>  #include "../../util/symbol.h"
>  #include "../../util/evsel.h"
>  #include "../../util/evlist.h"
>  #include <inttypes.h>
> -#include <pthread.h>
>  #include <linux/kernel.h>
>  #include <linux/string.h>
>  #include <linux/zalloc.h>
> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> index 700335cde618..25ded88801a3 100644
> --- a/tools/perf/ui/setup.c
> +++ b/tools/perf/ui/setup.c
> @@ -1,5 +1,4 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include <pthread.h>
>  #include <dlfcn.h>
>  #include <unistd.h>
>  
> @@ -8,7 +7,7 @@
>  #include "../util/hist.h"
>  #include "ui.h"
>  
> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> +struct mutex ui__lock;
>  void *perf_gtk_handle;
>  int use_browser = -1;
>  
> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>  
>  void setup_browser(bool fallback_to_pager)
>  {
> +	mutex_init(&ui__lock);
>  	if (use_browser < 2 && (!isatty(1) || dump_trace))
>  		use_browser = 0;
>  
> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
>  	default:
>  		break;
>  	}
> +	mutex_destroy(&ui__lock);

Looks like exit_browser() can be called even when setup_browser()
was never called.

Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
pthread won't notice.

>  }
> diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> index 298d6af82fdd..db4952f5990b 100644
> --- a/tools/perf/ui/tui/helpline.c
> +++ b/tools/perf/ui/tui/helpline.c
> @@ -2,7 +2,6 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -#include <pthread.h>
>  #include <linux/kernel.h>
>  #include <linux/string.h>
>  
> @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
>  	int ret;
>  	static int backlog;
>  
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	ret = vscnprintf(ui_helpline__last_msg + backlog,
>  			sizeof(ui_helpline__last_msg) - backlog, format, ap);
>  	backlog += ret;
> @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
>  		SLsmg_refresh();
>  		backlog = 0;
>  	}
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  
>  	return ret;
>  }
> diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> index 3d74af5a7ece..71b6c8d9474f 100644
> --- a/tools/perf/ui/tui/progress.c
> +++ b/tools/perf/ui/tui/progress.c
> @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
>  	}
>  
>  	ui__refresh_dimensions(false);
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	y = SLtt_Screen_Rows / 2 - 2;
>  	SLsmg_set_color(0);
>  	SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
>  	bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
>  	SLsmg_fill_region(y, 1, 1, bar, ' ');
>  	SLsmg_refresh();
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  static void tui_progress__finish(void)
> @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
>  		return;
>  
>  	ui__refresh_dimensions(false);
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	y = SLtt_Screen_Rows / 2 - 2;
>  	SLsmg_set_color(0);
>  	SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
>  	SLsmg_refresh();
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  static struct ui_progress_ops tui_progress__ops = {
> diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> index b1be59b4e2a4..a3b8c397c24d 100644
> --- a/tools/perf/ui/tui/setup.c
> +++ b/tools/perf/ui/tui/setup.c
> @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
>  {
>  	if (force || ui__need_resize) {
>  		ui__need_resize = 0;
> -		pthread_mutex_lock(&ui__lock);
> +		mutex_lock(&ui__lock);
>  		SLtt_get_screen_size();
>  		SLsmg_reinit_smg();
> -		pthread_mutex_unlock(&ui__lock);
> +		mutex_unlock(&ui__lock);
>  	}
>  }
>  
> @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
>  				    "Press any key...", 0);
>  
>  	SLtt_set_cursor_visibility(1);
> -	if (!pthread_mutex_trylock(&ui__lock)) {
> +	if (mutex_trylock(&ui__lock)) {
>  		SLsmg_refresh();
>  		SLsmg_reset_smg();
> -		pthread_mutex_unlock(&ui__lock);
> +		mutex_unlock(&ui__lock);
>  	}
>  	SLang_reset_tty();
>  	perf_error__unregister(&perf_tui_eops);
> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> index 0f562e2cb1e8..3c5174854ac8 100644
> --- a/tools/perf/ui/tui/util.c
> +++ b/tools/perf/ui/tui/util.c
> @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>  		t = sep + 1;
>  	}
>  
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  
>  	max_len += 2;
>  	nr_lines += 8;
> @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>  	SLsmg_write_nstring((char *)exit_msg, max_len);
>  	SLsmg_refresh();
>  
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  
>  	x += 2;
>  	len = 0;
>  	key = ui__getch(delay_secs);
>  	while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> -		pthread_mutex_lock(&ui__lock);
> +		mutex_lock(&ui__lock);
>  
>  		if (key == K_BKSPC) {
>  			if (len == 0) {
> -				pthread_mutex_unlock(&ui__lock);
> +				mutex_unlock(&ui__lock);
>  				goto next_key;
>  			}
>  			SLsmg_gotorc(y, x + --len);
> @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>  		}
>  		SLsmg_refresh();
>  
> -		pthread_mutex_unlock(&ui__lock);
> +		mutex_unlock(&ui__lock);
>  
>  		/* XXX more graceful overflow handling needed */
>  		if (len == sizeof(buf) - 1) {
> @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
>  
>  void ui__info_window(const char *title, const char *text)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui__info_window(title, text, NULL);
>  	SLsmg_refresh();
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  }
>  
>  int ui__question_window(const char *title, const char *text,
>  			const char *exit_msg, int delay_secs)
>  {
> -	pthread_mutex_lock(&ui__lock);
> +	mutex_lock(&ui__lock);
>  	__ui__info_window(title, text, exit_msg);
>  	SLsmg_refresh();
> -	pthread_mutex_unlock(&ui__lock);
> +	mutex_unlock(&ui__lock);
>  	return ui__getch(delay_secs);
>  }
>  
> diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> --- a/tools/perf/ui/ui.h
> +++ b/tools/perf/ui/ui.h
> @@ -2,11 +2,11 @@
>  #ifndef _PERF_UI_H_
>  #define _PERF_UI_H_ 1
>  
> -#include <pthread.h>
> +#include "../util/mutex.h"
>  #include <stdbool.h>
>  #include <linux/compiler.h>
>  
> -extern pthread_mutex_t ui__lock;
> +extern struct mutex ui__lock;
>  extern void *perf_gtk_handle;
>  
>  extern int use_browser;

Ian Rogers Aug. 26, 2022, 4:01 p.m. UTC | #3

On Fri, Aug 26, 2022 at 3:11 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 24/08/22 18:38, Ian Rogers wrote:
> > Switch to the use of mutex wrappers that provide better error checking.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/ui/browser.c           | 20 ++++++++++----------
> >  tools/perf/ui/browsers/annotate.c |  2 +-
>
> Other changes to tools/perf/ui/browsers/annotate.c are in patch 12

Yes, these changes relate to the ui__lock and the patch 12 changes
relate to annotation lock. The grouping was done in this way so that
the patches would build independently.

Thanks,
Ian

> >  tools/perf/ui/setup.c             |  5 +++--
> >  tools/perf/ui/tui/helpline.c      |  5 ++---
> >  tools/perf/ui/tui/progress.c      |  8 ++++----
> >  tools/perf/ui/tui/setup.c         |  8 ++++----
> >  tools/perf/ui/tui/util.c          | 18 +++++++++---------
> >  tools/perf/ui/ui.h                |  4 ++--
> >  8 files changed, 35 insertions(+), 35 deletions(-)
> >
> > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > index fa5bd5c20e96..78fb01d6ad63 100644
> > --- a/tools/perf/ui/browser.c
> > +++ b/tools/perf/ui/browser.c
> > @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
> >
> >  void ui_browser__show_title(struct ui_browser *browser, const char *title)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui_browser__show_title(browser, title);
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  int ui_browser__show(struct ui_browser *browser, const char *title,
> > @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >
> >       browser->refresh_dimensions(browser);
> >
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui_browser__show_title(browser, title);
> >
> >       browser->title = title;
> > @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >       va_end(ap);
> >       if (err > 0)
> >               ui_helpline__push(browser->helpline);
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >       return err ? 0 : -1;
> >  }
> >
> >  void ui_browser__hide(struct ui_browser *browser)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       ui_helpline__pop();
> >       zfree(&browser->helpline);
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  static void ui_browser__scrollbar_set(struct ui_browser *browser)
> > @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
> >
> >  int ui_browser__refresh(struct ui_browser *browser)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui_browser__refresh(browser);
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >
> >       return 0;
> >  }
> > @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
> >       while (1) {
> >               off_t offset;
> >
> > -             pthread_mutex_lock(&ui__lock);
> > +             mutex_lock(&ui__lock);
> >               err = __ui_browser__refresh(browser);
> >               SLsmg_refresh();
> > -             pthread_mutex_unlock(&ui__lock);
> > +             mutex_unlock(&ui__lock);
> >               if (err < 0)
> >                       break;
> >
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 44ba900828f6..b8747e8dd9ea 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -8,11 +8,11 @@
> >  #include "../../util/hist.h"
> >  #include "../../util/sort.h"
> >  #include "../../util/map.h"
> > +#include "../../util/mutex.h"
> >  #include "../../util/symbol.h"
> >  #include "../../util/evsel.h"
> >  #include "../../util/evlist.h"
> >  #include <inttypes.h>
> > -#include <pthread.h>
> >  #include <linux/kernel.h>
> >  #include <linux/string.h>
> >  #include <linux/zalloc.h>
> > diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> > index 700335cde618..25ded88801a3 100644
> > --- a/tools/perf/ui/setup.c
> > +++ b/tools/perf/ui/setup.c
> > @@ -1,5 +1,4 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > -#include <pthread.h>
> >  #include <dlfcn.h>
> >  #include <unistd.h>
> >
> > @@ -8,7 +7,7 @@
> >  #include "../util/hist.h"
> >  #include "ui.h"
> >
> > -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> > +struct mutex ui__lock;
> >  void *perf_gtk_handle;
> >  int use_browser = -1;
> >
> > @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
> >
> >  void setup_browser(bool fallback_to_pager)
> >  {
> > +     mutex_init(&ui__lock);
> >       if (use_browser < 2 && (!isatty(1) || dump_trace))
> >               use_browser = 0;
> >
> > @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
> >       default:
> >               break;
> >       }
> > +     mutex_destroy(&ui__lock);
> >  }
> > diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> > index 298d6af82fdd..db4952f5990b 100644
> > --- a/tools/perf/ui/tui/helpline.c
> > +++ b/tools/perf/ui/tui/helpline.c
> > @@ -2,7 +2,6 @@
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > -#include <pthread.h>
> >  #include <linux/kernel.h>
> >  #include <linux/string.h>
> >
> > @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> >       int ret;
> >       static int backlog;
> >
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       ret = vscnprintf(ui_helpline__last_msg + backlog,
> >                       sizeof(ui_helpline__last_msg) - backlog, format, ap);
> >       backlog += ret;
> > @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> >               SLsmg_refresh();
> >               backlog = 0;
> >       }
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >
> >       return ret;
> >  }
> > diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> > index 3d74af5a7ece..71b6c8d9474f 100644
> > --- a/tools/perf/ui/tui/progress.c
> > +++ b/tools/perf/ui/tui/progress.c
> > @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
> >       }
> >
> >       ui__refresh_dimensions(false);
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       y = SLtt_Screen_Rows / 2 - 2;
> >       SLsmg_set_color(0);
> >       SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> > @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
> >       bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
> >       SLsmg_fill_region(y, 1, 1, bar, ' ');
> >       SLsmg_refresh();
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  static void tui_progress__finish(void)
> > @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
> >               return;
> >
> >       ui__refresh_dimensions(false);
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       y = SLtt_Screen_Rows / 2 - 2;
> >       SLsmg_set_color(0);
> >       SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
> >       SLsmg_refresh();
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  static struct ui_progress_ops tui_progress__ops = {
> > diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> > index b1be59b4e2a4..a3b8c397c24d 100644
> > --- a/tools/perf/ui/tui/setup.c
> > +++ b/tools/perf/ui/tui/setup.c
> > @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
> >  {
> >       if (force || ui__need_resize) {
> >               ui__need_resize = 0;
> > -             pthread_mutex_lock(&ui__lock);
> > +             mutex_lock(&ui__lock);
> >               SLtt_get_screen_size();
> >               SLsmg_reinit_smg();
> > -             pthread_mutex_unlock(&ui__lock);
> > +             mutex_unlock(&ui__lock);
> >       }
> >  }
> >
> > @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
> >                                   "Press any key...", 0);
> >
> >       SLtt_set_cursor_visibility(1);
> > -     if (!pthread_mutex_trylock(&ui__lock)) {
> > +     if (mutex_trylock(&ui__lock)) {
> >               SLsmg_refresh();
> >               SLsmg_reset_smg();
> > -             pthread_mutex_unlock(&ui__lock);
> > +             mutex_unlock(&ui__lock);
> >       }
> >       SLang_reset_tty();
> >       perf_error__unregister(&perf_tui_eops);
> > diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> > index 0f562e2cb1e8..3c5174854ac8 100644
> > --- a/tools/perf/ui/tui/util.c
> > +++ b/tools/perf/ui/tui/util.c
> > @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >               t = sep + 1;
> >       }
> >
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >
> >       max_len += 2;
> >       nr_lines += 8;
> > @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >       SLsmg_write_nstring((char *)exit_msg, max_len);
> >       SLsmg_refresh();
> >
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >
> >       x += 2;
> >       len = 0;
> >       key = ui__getch(delay_secs);
> >       while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> > -             pthread_mutex_lock(&ui__lock);
> > +             mutex_lock(&ui__lock);
> >
> >               if (key == K_BKSPC) {
> >                       if (len == 0) {
> > -                             pthread_mutex_unlock(&ui__lock);
> > +                             mutex_unlock(&ui__lock);
> >                               goto next_key;
> >                       }
> >                       SLsmg_gotorc(y, x + --len);
> > @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >               }
> >               SLsmg_refresh();
> >
> > -             pthread_mutex_unlock(&ui__lock);
> > +             mutex_unlock(&ui__lock);
> >
> >               /* XXX more graceful overflow handling needed */
> >               if (len == sizeof(buf) - 1) {
> > @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
> >
> >  void ui__info_window(const char *title, const char *text)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui__info_window(title, text, NULL);
> >       SLsmg_refresh();
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  int ui__question_window(const char *title, const char *text,
> >                       const char *exit_msg, int delay_secs)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui__info_window(title, text, exit_msg);
> >       SLsmg_refresh();
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >       return ui__getch(delay_secs);
> >  }
> >
> > diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> > index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> > --- a/tools/perf/ui/ui.h
> > +++ b/tools/perf/ui/ui.h
> > @@ -2,11 +2,11 @@
> >  #ifndef _PERF_UI_H_
> >  #define _PERF_UI_H_ 1
> >
> > -#include <pthread.h>
> > +#include "../util/mutex.h"
> >  #include <stdbool.h>
> >  #include <linux/compiler.h>
> >
> > -extern pthread_mutex_t ui__lock;
> > +extern struct mutex ui__lock;
> >  extern void *perf_gtk_handle;
> >
> >  extern int use_browser;
>

Ian Rogers Aug. 26, 2022, 4:02 p.m. UTC | #4

On Fri, Aug 26, 2022 at 3:24 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 24/08/22 18:38, Ian Rogers wrote:
> > Switch to the use of mutex wrappers that provide better error checking.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/ui/browser.c           | 20 ++++++++++----------
> >  tools/perf/ui/browsers/annotate.c |  2 +-
> >  tools/perf/ui/setup.c             |  5 +++--
> >  tools/perf/ui/tui/helpline.c      |  5 ++---
> >  tools/perf/ui/tui/progress.c      |  8 ++++----
> >  tools/perf/ui/tui/setup.c         |  8 ++++----
> >  tools/perf/ui/tui/util.c          | 18 +++++++++---------
> >  tools/perf/ui/ui.h                |  4 ++--
> >  8 files changed, 35 insertions(+), 35 deletions(-)
> >
> > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > index fa5bd5c20e96..78fb01d6ad63 100644
> > --- a/tools/perf/ui/browser.c
> > +++ b/tools/perf/ui/browser.c
> > @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
> >
> >  void ui_browser__show_title(struct ui_browser *browser, const char *title)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui_browser__show_title(browser, title);
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  int ui_browser__show(struct ui_browser *browser, const char *title,
> > @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >
> >       browser->refresh_dimensions(browser);
> >
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui_browser__show_title(browser, title);
> >
> >       browser->title = title;
> > @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >       va_end(ap);
> >       if (err > 0)
> >               ui_helpline__push(browser->helpline);
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >       return err ? 0 : -1;
> >  }
> >
> >  void ui_browser__hide(struct ui_browser *browser)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       ui_helpline__pop();
> >       zfree(&browser->helpline);
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  static void ui_browser__scrollbar_set(struct ui_browser *browser)
> > @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
> >
> >  int ui_browser__refresh(struct ui_browser *browser)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui_browser__refresh(browser);
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >
> >       return 0;
> >  }
> > @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
> >       while (1) {
> >               off_t offset;
> >
> > -             pthread_mutex_lock(&ui__lock);
> > +             mutex_lock(&ui__lock);
> >               err = __ui_browser__refresh(browser);
> >               SLsmg_refresh();
> > -             pthread_mutex_unlock(&ui__lock);
> > +             mutex_unlock(&ui__lock);
> >               if (err < 0)
> >                       break;
> >
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 44ba900828f6..b8747e8dd9ea 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -8,11 +8,11 @@
> >  #include "../../util/hist.h"
> >  #include "../../util/sort.h"
> >  #include "../../util/map.h"
> > +#include "../../util/mutex.h"
> >  #include "../../util/symbol.h"
> >  #include "../../util/evsel.h"
> >  #include "../../util/evlist.h"
> >  #include <inttypes.h>
> > -#include <pthread.h>
> >  #include <linux/kernel.h>
> >  #include <linux/string.h>
> >  #include <linux/zalloc.h>
> > diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> > index 700335cde618..25ded88801a3 100644
> > --- a/tools/perf/ui/setup.c
> > +++ b/tools/perf/ui/setup.c
> > @@ -1,5 +1,4 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > -#include <pthread.h>
> >  #include <dlfcn.h>
> >  #include <unistd.h>
> >
> > @@ -8,7 +7,7 @@
> >  #include "../util/hist.h"
> >  #include "ui.h"
> >
> > -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> > +struct mutex ui__lock;
> >  void *perf_gtk_handle;
> >  int use_browser = -1;
> >
> > @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
> >
> >  void setup_browser(bool fallback_to_pager)
> >  {
> > +     mutex_init(&ui__lock);
> >       if (use_browser < 2 && (!isatty(1) || dump_trace))
> >               use_browser = 0;
> >
> > @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
> >       default:
> >               break;
> >       }
> > +     mutex_destroy(&ui__lock);
>
> Looks like exit_browser() can be called even when setup_browser()
> was never called.
>
> Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
> pthread won't notice.

Memory sanitizer will notice some cases of this and so I didn't want
to code defensively around exit being called ahead of setup.

Thanks,
Ian

> >  }
> > diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> > index 298d6af82fdd..db4952f5990b 100644
> > --- a/tools/perf/ui/tui/helpline.c
> > +++ b/tools/perf/ui/tui/helpline.c
> > @@ -2,7 +2,6 @@
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > -#include <pthread.h>
> >  #include <linux/kernel.h>
> >  #include <linux/string.h>
> >
> > @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> >       int ret;
> >       static int backlog;
> >
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       ret = vscnprintf(ui_helpline__last_msg + backlog,
> >                       sizeof(ui_helpline__last_msg) - backlog, format, ap);
> >       backlog += ret;
> > @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> >               SLsmg_refresh();
> >               backlog = 0;
> >       }
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >
> >       return ret;
> >  }
> > diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> > index 3d74af5a7ece..71b6c8d9474f 100644
> > --- a/tools/perf/ui/tui/progress.c
> > +++ b/tools/perf/ui/tui/progress.c
> > @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
> >       }
> >
> >       ui__refresh_dimensions(false);
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       y = SLtt_Screen_Rows / 2 - 2;
> >       SLsmg_set_color(0);
> >       SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> > @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
> >       bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
> >       SLsmg_fill_region(y, 1, 1, bar, ' ');
> >       SLsmg_refresh();
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  static void tui_progress__finish(void)
> > @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
> >               return;
> >
> >       ui__refresh_dimensions(false);
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       y = SLtt_Screen_Rows / 2 - 2;
> >       SLsmg_set_color(0);
> >       SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
> >       SLsmg_refresh();
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  static struct ui_progress_ops tui_progress__ops = {
> > diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> > index b1be59b4e2a4..a3b8c397c24d 100644
> > --- a/tools/perf/ui/tui/setup.c
> > +++ b/tools/perf/ui/tui/setup.c
> > @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
> >  {
> >       if (force || ui__need_resize) {
> >               ui__need_resize = 0;
> > -             pthread_mutex_lock(&ui__lock);
> > +             mutex_lock(&ui__lock);
> >               SLtt_get_screen_size();
> >               SLsmg_reinit_smg();
> > -             pthread_mutex_unlock(&ui__lock);
> > +             mutex_unlock(&ui__lock);
> >       }
> >  }
> >
> > @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
> >                                   "Press any key...", 0);
> >
> >       SLtt_set_cursor_visibility(1);
> > -     if (!pthread_mutex_trylock(&ui__lock)) {
> > +     if (mutex_trylock(&ui__lock)) {
> >               SLsmg_refresh();
> >               SLsmg_reset_smg();
> > -             pthread_mutex_unlock(&ui__lock);
> > +             mutex_unlock(&ui__lock);
> >       }
> >       SLang_reset_tty();
> >       perf_error__unregister(&perf_tui_eops);
> > diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> > index 0f562e2cb1e8..3c5174854ac8 100644
> > --- a/tools/perf/ui/tui/util.c
> > +++ b/tools/perf/ui/tui/util.c
> > @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >               t = sep + 1;
> >       }
> >
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >
> >       max_len += 2;
> >       nr_lines += 8;
> > @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >       SLsmg_write_nstring((char *)exit_msg, max_len);
> >       SLsmg_refresh();
> >
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >
> >       x += 2;
> >       len = 0;
> >       key = ui__getch(delay_secs);
> >       while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> > -             pthread_mutex_lock(&ui__lock);
> > +             mutex_lock(&ui__lock);
> >
> >               if (key == K_BKSPC) {
> >                       if (len == 0) {
> > -                             pthread_mutex_unlock(&ui__lock);
> > +                             mutex_unlock(&ui__lock);
> >                               goto next_key;
> >                       }
> >                       SLsmg_gotorc(y, x + --len);
> > @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >               }
> >               SLsmg_refresh();
> >
> > -             pthread_mutex_unlock(&ui__lock);
> > +             mutex_unlock(&ui__lock);
> >
> >               /* XXX more graceful overflow handling needed */
> >               if (len == sizeof(buf) - 1) {
> > @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
> >
> >  void ui__info_window(const char *title, const char *text)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui__info_window(title, text, NULL);
> >       SLsmg_refresh();
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >  }
> >
> >  int ui__question_window(const char *title, const char *text,
> >                       const char *exit_msg, int delay_secs)
> >  {
> > -     pthread_mutex_lock(&ui__lock);
> > +     mutex_lock(&ui__lock);
> >       __ui__info_window(title, text, exit_msg);
> >       SLsmg_refresh();
> > -     pthread_mutex_unlock(&ui__lock);
> > +     mutex_unlock(&ui__lock);
> >       return ui__getch(delay_secs);
> >  }
> >
> > diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> > index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> > --- a/tools/perf/ui/ui.h
> > +++ b/tools/perf/ui/ui.h
> > @@ -2,11 +2,11 @@
> >  #ifndef _PERF_UI_H_
> >  #define _PERF_UI_H_ 1
> >
> > -#include <pthread.h>
> > +#include "../util/mutex.h"
> >  #include <stdbool.h>
> >  #include <linux/compiler.h>
> >
> > -extern pthread_mutex_t ui__lock;
> > +extern struct mutex ui__lock;
> >  extern void *perf_gtk_handle;
> >
> >  extern int use_browser;
>

Adrian Hunter Aug. 26, 2022, 5:22 p.m. UTC | #5

On 26/08/22 19:02, Ian Rogers wrote:
> On Fri, Aug 26, 2022 at 3:24 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 24/08/22 18:38, Ian Rogers wrote:
>>> Switch to the use of mutex wrappers that provide better error checking.
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>  tools/perf/ui/browser.c           | 20 ++++++++++----------
>>>  tools/perf/ui/browsers/annotate.c |  2 +-
>>>  tools/perf/ui/setup.c             |  5 +++--
>>>  tools/perf/ui/tui/helpline.c      |  5 ++---
>>>  tools/perf/ui/tui/progress.c      |  8 ++++----
>>>  tools/perf/ui/tui/setup.c         |  8 ++++----
>>>  tools/perf/ui/tui/util.c          | 18 +++++++++---------
>>>  tools/perf/ui/ui.h                |  4 ++--
>>>  8 files changed, 35 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>>> index fa5bd5c20e96..78fb01d6ad63 100644
>>> --- a/tools/perf/ui/browser.c
>>> +++ b/tools/perf/ui/browser.c
>>> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
>>>
>>>  void ui_browser__show_title(struct ui_browser *browser, const char *title)
>>>  {
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>       __ui_browser__show_title(browser, title);
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>  }
>>>
>>>  int ui_browser__show(struct ui_browser *browser, const char *title,
>>> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>>
>>>       browser->refresh_dimensions(browser);
>>>
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>       __ui_browser__show_title(browser, title);
>>>
>>>       browser->title = title;
>>> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>>       va_end(ap);
>>>       if (err > 0)
>>>               ui_helpline__push(browser->helpline);
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>       return err ? 0 : -1;
>>>  }
>>>
>>>  void ui_browser__hide(struct ui_browser *browser)
>>>  {
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>       ui_helpline__pop();
>>>       zfree(&browser->helpline);
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>  }
>>>
>>>  static void ui_browser__scrollbar_set(struct ui_browser *browser)
>>> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
>>>
>>>  int ui_browser__refresh(struct ui_browser *browser)
>>>  {
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>       __ui_browser__refresh(browser);
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>
>>>       return 0;
>>>  }
>>> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
>>>       while (1) {
>>>               off_t offset;
>>>
>>> -             pthread_mutex_lock(&ui__lock);
>>> +             mutex_lock(&ui__lock);
>>>               err = __ui_browser__refresh(browser);
>>>               SLsmg_refresh();
>>> -             pthread_mutex_unlock(&ui__lock);
>>> +             mutex_unlock(&ui__lock);
>>>               if (err < 0)
>>>                       break;
>>>
>>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
>>> index 44ba900828f6..b8747e8dd9ea 100644
>>> --- a/tools/perf/ui/browsers/annotate.c
>>> +++ b/tools/perf/ui/browsers/annotate.c
>>> @@ -8,11 +8,11 @@
>>>  #include "../../util/hist.h"
>>>  #include "../../util/sort.h"
>>>  #include "../../util/map.h"
>>> +#include "../../util/mutex.h"
>>>  #include "../../util/symbol.h"
>>>  #include "../../util/evsel.h"
>>>  #include "../../util/evlist.h"
>>>  #include <inttypes.h>
>>> -#include <pthread.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/string.h>
>>>  #include <linux/zalloc.h>
>>> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
>>> index 700335cde618..25ded88801a3 100644
>>> --- a/tools/perf/ui/setup.c
>>> +++ b/tools/perf/ui/setup.c
>>> @@ -1,5 +1,4 @@
>>>  // SPDX-License-Identifier: GPL-2.0
>>> -#include <pthread.h>
>>>  #include <dlfcn.h>
>>>  #include <unistd.h>
>>>
>>> @@ -8,7 +7,7 @@
>>>  #include "../util/hist.h"
>>>  #include "ui.h"
>>>
>>> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
>>> +struct mutex ui__lock;
>>>  void *perf_gtk_handle;
>>>  int use_browser = -1;
>>>
>>> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>>>
>>>  void setup_browser(bool fallback_to_pager)
>>>  {
>>> +     mutex_init(&ui__lock);
>>>       if (use_browser < 2 && (!isatty(1) || dump_trace))
>>>               use_browser = 0;
>>>
>>> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
>>>       default:
>>>               break;
>>>       }
>>> +     mutex_destroy(&ui__lock);
>>
>> Looks like exit_browser() can be called even when setup_browser()
>> was never called.
>>
>> Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
>> pthread won't notice.
> 
> Memory sanitizer will notice some cases of this and so I didn't want
> to code defensively around exit being called ahead of setup.

I am not sure you understood.

As I wrote, exit_browser() can be called even when setup_browser()
was never called, so it is not defensive programming, it is necessary
programming that you only get away without doing because
PTHREAD_MUTEX_INITIALIZER is all zeros.

> 
> Thanks,
> Ian
> 
>>>  }
>>> diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
>>> index 298d6af82fdd..db4952f5990b 100644
>>> --- a/tools/perf/ui/tui/helpline.c
>>> +++ b/tools/perf/ui/tui/helpline.c
>>> @@ -2,7 +2,6 @@
>>>  #include <stdio.h>
>>>  #include <stdlib.h>
>>>  #include <string.h>
>>> -#include <pthread.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/string.h>
>>>
>>> @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
>>>       int ret;
>>>       static int backlog;
>>>
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>       ret = vscnprintf(ui_helpline__last_msg + backlog,
>>>                       sizeof(ui_helpline__last_msg) - backlog, format, ap);
>>>       backlog += ret;
>>> @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
>>>               SLsmg_refresh();
>>>               backlog = 0;
>>>       }
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>
>>>       return ret;
>>>  }
>>> diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
>>> index 3d74af5a7ece..71b6c8d9474f 100644
>>> --- a/tools/perf/ui/tui/progress.c
>>> +++ b/tools/perf/ui/tui/progress.c
>>> @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
>>>       }
>>>
>>>       ui__refresh_dimensions(false);
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>       y = SLtt_Screen_Rows / 2 - 2;
>>>       SLsmg_set_color(0);
>>>       SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
>>> @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
>>>       bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
>>>       SLsmg_fill_region(y, 1, 1, bar, ' ');
>>>       SLsmg_refresh();
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>  }
>>>
>>>  static void tui_progress__finish(void)
>>> @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
>>>               return;
>>>
>>>       ui__refresh_dimensions(false);
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>       y = SLtt_Screen_Rows / 2 - 2;
>>>       SLsmg_set_color(0);
>>>       SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
>>>       SLsmg_refresh();
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>  }
>>>
>>>  static struct ui_progress_ops tui_progress__ops = {
>>> diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
>>> index b1be59b4e2a4..a3b8c397c24d 100644
>>> --- a/tools/perf/ui/tui/setup.c
>>> +++ b/tools/perf/ui/tui/setup.c
>>> @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
>>>  {
>>>       if (force || ui__need_resize) {
>>>               ui__need_resize = 0;
>>> -             pthread_mutex_lock(&ui__lock);
>>> +             mutex_lock(&ui__lock);
>>>               SLtt_get_screen_size();
>>>               SLsmg_reinit_smg();
>>> -             pthread_mutex_unlock(&ui__lock);
>>> +             mutex_unlock(&ui__lock);
>>>       }
>>>  }
>>>
>>> @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
>>>                                   "Press any key...", 0);
>>>
>>>       SLtt_set_cursor_visibility(1);
>>> -     if (!pthread_mutex_trylock(&ui__lock)) {
>>> +     if (mutex_trylock(&ui__lock)) {
>>>               SLsmg_refresh();
>>>               SLsmg_reset_smg();
>>> -             pthread_mutex_unlock(&ui__lock);
>>> +             mutex_unlock(&ui__lock);
>>>       }
>>>       SLang_reset_tty();
>>>       perf_error__unregister(&perf_tui_eops);
>>> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
>>> index 0f562e2cb1e8..3c5174854ac8 100644
>>> --- a/tools/perf/ui/tui/util.c
>>> +++ b/tools/perf/ui/tui/util.c
>>> @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>>>               t = sep + 1;
>>>       }
>>>
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>
>>>       max_len += 2;
>>>       nr_lines += 8;
>>> @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>>>       SLsmg_write_nstring((char *)exit_msg, max_len);
>>>       SLsmg_refresh();
>>>
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>
>>>       x += 2;
>>>       len = 0;
>>>       key = ui__getch(delay_secs);
>>>       while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
>>> -             pthread_mutex_lock(&ui__lock);
>>> +             mutex_lock(&ui__lock);
>>>
>>>               if (key == K_BKSPC) {
>>>                       if (len == 0) {
>>> -                             pthread_mutex_unlock(&ui__lock);
>>> +                             mutex_unlock(&ui__lock);
>>>                               goto next_key;
>>>                       }
>>>                       SLsmg_gotorc(y, x + --len);
>>> @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
>>>               }
>>>               SLsmg_refresh();
>>>
>>> -             pthread_mutex_unlock(&ui__lock);
>>> +             mutex_unlock(&ui__lock);
>>>
>>>               /* XXX more graceful overflow handling needed */
>>>               if (len == sizeof(buf) - 1) {
>>> @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
>>>
>>>  void ui__info_window(const char *title, const char *text)
>>>  {
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>       __ui__info_window(title, text, NULL);
>>>       SLsmg_refresh();
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>  }
>>>
>>>  int ui__question_window(const char *title, const char *text,
>>>                       const char *exit_msg, int delay_secs)
>>>  {
>>> -     pthread_mutex_lock(&ui__lock);
>>> +     mutex_lock(&ui__lock);
>>>       __ui__info_window(title, text, exit_msg);
>>>       SLsmg_refresh();
>>> -     pthread_mutex_unlock(&ui__lock);
>>> +     mutex_unlock(&ui__lock);
>>>       return ui__getch(delay_secs);
>>>  }
>>>
>>> diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
>>> index 9b6fdf06e1d2..99f8d2fe9bc5 100644
>>> --- a/tools/perf/ui/ui.h
>>> +++ b/tools/perf/ui/ui.h
>>> @@ -2,11 +2,11 @@
>>>  #ifndef _PERF_UI_H_
>>>  #define _PERF_UI_H_ 1
>>>
>>> -#include <pthread.h>
>>> +#include "../util/mutex.h"
>>>  #include <stdbool.h>
>>>  #include <linux/compiler.h>
>>>
>>> -extern pthread_mutex_t ui__lock;
>>> +extern struct mutex ui__lock;
>>>  extern void *perf_gtk_handle;
>>>
>>>  extern int use_browser;
>>

Ian Rogers Aug. 26, 2022, 5:45 p.m. UTC | #6

On Fri, Aug 26, 2022 at 10:22 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 26/08/22 19:02, Ian Rogers wrote:
> > On Fri, Aug 26, 2022 at 3:24 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 24/08/22 18:38, Ian Rogers wrote:
> >>> Switch to the use of mutex wrappers that provide better error checking.
> >>>
> >>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>> ---
> >>>  tools/perf/ui/browser.c           | 20 ++++++++++----------
> >>>  tools/perf/ui/browsers/annotate.c |  2 +-
> >>>  tools/perf/ui/setup.c             |  5 +++--
> >>>  tools/perf/ui/tui/helpline.c      |  5 ++---
> >>>  tools/perf/ui/tui/progress.c      |  8 ++++----
> >>>  tools/perf/ui/tui/setup.c         |  8 ++++----
> >>>  tools/perf/ui/tui/util.c          | 18 +++++++++---------
> >>>  tools/perf/ui/ui.h                |  4 ++--
> >>>  8 files changed, 35 insertions(+), 35 deletions(-)
> >>>
> >>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> >>> index fa5bd5c20e96..78fb01d6ad63 100644
> >>> --- a/tools/perf/ui/browser.c
> >>> +++ b/tools/perf/ui/browser.c
> >>> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
> >>>
> >>>  void ui_browser__show_title(struct ui_browser *browser, const char *title)
> >>>  {
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>       __ui_browser__show_title(browser, title);
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>  }
> >>>
> >>>  int ui_browser__show(struct ui_browser *browser, const char *title,
> >>> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >>>
> >>>       browser->refresh_dimensions(browser);
> >>>
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>       __ui_browser__show_title(browser, title);
> >>>
> >>>       browser->title = title;
> >>> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >>>       va_end(ap);
> >>>       if (err > 0)
> >>>               ui_helpline__push(browser->helpline);
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>       return err ? 0 : -1;
> >>>  }
> >>>
> >>>  void ui_browser__hide(struct ui_browser *browser)
> >>>  {
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>       ui_helpline__pop();
> >>>       zfree(&browser->helpline);
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>  }
> >>>
> >>>  static void ui_browser__scrollbar_set(struct ui_browser *browser)
> >>> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
> >>>
> >>>  int ui_browser__refresh(struct ui_browser *browser)
> >>>  {
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>       __ui_browser__refresh(browser);
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>
> >>>       return 0;
> >>>  }
> >>> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
> >>>       while (1) {
> >>>               off_t offset;
> >>>
> >>> -             pthread_mutex_lock(&ui__lock);
> >>> +             mutex_lock(&ui__lock);
> >>>               err = __ui_browser__refresh(browser);
> >>>               SLsmg_refresh();
> >>> -             pthread_mutex_unlock(&ui__lock);
> >>> +             mutex_unlock(&ui__lock);
> >>>               if (err < 0)
> >>>                       break;
> >>>
> >>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> >>> index 44ba900828f6..b8747e8dd9ea 100644
> >>> --- a/tools/perf/ui/browsers/annotate.c
> >>> +++ b/tools/perf/ui/browsers/annotate.c
> >>> @@ -8,11 +8,11 @@
> >>>  #include "../../util/hist.h"
> >>>  #include "../../util/sort.h"
> >>>  #include "../../util/map.h"
> >>> +#include "../../util/mutex.h"
> >>>  #include "../../util/symbol.h"
> >>>  #include "../../util/evsel.h"
> >>>  #include "../../util/evlist.h"
> >>>  #include <inttypes.h>
> >>> -#include <pthread.h>
> >>>  #include <linux/kernel.h>
> >>>  #include <linux/string.h>
> >>>  #include <linux/zalloc.h>
> >>> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> >>> index 700335cde618..25ded88801a3 100644
> >>> --- a/tools/perf/ui/setup.c
> >>> +++ b/tools/perf/ui/setup.c
> >>> @@ -1,5 +1,4 @@
> >>>  // SPDX-License-Identifier: GPL-2.0
> >>> -#include <pthread.h>
> >>>  #include <dlfcn.h>
> >>>  #include <unistd.h>
> >>>
> >>> @@ -8,7 +7,7 @@
> >>>  #include "../util/hist.h"
> >>>  #include "ui.h"
> >>>
> >>> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
> >>> +struct mutex ui__lock;
> >>>  void *perf_gtk_handle;
> >>>  int use_browser = -1;
> >>>
> >>> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
> >>>
> >>>  void setup_browser(bool fallback_to_pager)
> >>>  {
> >>> +     mutex_init(&ui__lock);
> >>>       if (use_browser < 2 && (!isatty(1) || dump_trace))
> >>>               use_browser = 0;
> >>>
> >>> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
> >>>       default:
> >>>               break;
> >>>       }
> >>> +     mutex_destroy(&ui__lock);
> >>
> >> Looks like exit_browser() can be called even when setup_browser()
> >> was never called.
> >>
> >> Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
> >> pthread won't notice.
> >
> > Memory sanitizer will notice some cases of this and so I didn't want
> > to code defensively around exit being called ahead of setup.
>
> I am not sure you understood.
>
> As I wrote, exit_browser() can be called even when setup_browser()
> was never called, so it is not defensive programming, it is necessary
> programming that you only get away without doing because
> PTHREAD_MUTEX_INITIALIZER is all zeros.

Why are we here:
1) there is a memory leak
2) I fix the memory and trigger a use after free
3) I invent a reference count checker, use it to fix the memory leak,
use after free and missing locks - the patch for this in 10s of lines
long
4) when adding the lock fixes I defensively add error checking to the
mutex involved - mainly because I was scared I could introduce a
deadlock
5) I get asked to generalize this
6) GSoC contributor picks up and puts this down
7) I pull together the contributor's work
8) I get asked to turn a search and replace 4 patch fix into an unwieldy patch
9) I worry about the sanity of the change and add lock checking from clang
10) I end up trying to fix perf-sched who for some reason thought it
was perfectly valid to have threads blocked on mutexes that were
deallocated on the stack.
11) the UI code was written with a view that exiting something not
setup somehow made sense

I am drawing a line at fixing perf sched and the UI code. We can drop
this patch and keep things as a pthread_mutex_t, similarly for
perf-sched. I have gone about as far as I'm prepared to for the sake
of a 10s of line memory leak fix. Some private thoughts are, it would
be useful if review comments could be constructive, hey do this not
that, and not simply commenting on change or trying to shoehorn vast
amounts of tech debt clean up onto simple fixes.

Thanks,
Ian

> >
> > Thanks,
> > Ian
> >
> >>>  }
> >>> diff --git a/tools/perf/ui/tui/helpline.c b/tools/perf/ui/tui/helpline.c
> >>> index 298d6af82fdd..db4952f5990b 100644
> >>> --- a/tools/perf/ui/tui/helpline.c
> >>> +++ b/tools/perf/ui/tui/helpline.c
> >>> @@ -2,7 +2,6 @@
> >>>  #include <stdio.h>
> >>>  #include <stdlib.h>
> >>>  #include <string.h>
> >>> -#include <pthread.h>
> >>>  #include <linux/kernel.h>
> >>>  #include <linux/string.h>
> >>>
> >>> @@ -33,7 +32,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> >>>       int ret;
> >>>       static int backlog;
> >>>
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>       ret = vscnprintf(ui_helpline__last_msg + backlog,
> >>>                       sizeof(ui_helpline__last_msg) - backlog, format, ap);
> >>>       backlog += ret;
> >>> @@ -45,7 +44,7 @@ static int tui_helpline__show(const char *format, va_list ap)
> >>>               SLsmg_refresh();
> >>>               backlog = 0;
> >>>       }
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>
> >>>       return ret;
> >>>  }
> >>> diff --git a/tools/perf/ui/tui/progress.c b/tools/perf/ui/tui/progress.c
> >>> index 3d74af5a7ece..71b6c8d9474f 100644
> >>> --- a/tools/perf/ui/tui/progress.c
> >>> +++ b/tools/perf/ui/tui/progress.c
> >>> @@ -45,7 +45,7 @@ static void tui_progress__update(struct ui_progress *p)
> >>>       }
> >>>
> >>>       ui__refresh_dimensions(false);
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>       y = SLtt_Screen_Rows / 2 - 2;
> >>>       SLsmg_set_color(0);
> >>>       SLsmg_draw_box(y, 0, 3, SLtt_Screen_Cols);
> >>> @@ -56,7 +56,7 @@ static void tui_progress__update(struct ui_progress *p)
> >>>       bar = ((SLtt_Screen_Cols - 2) * p->curr) / p->total;
> >>>       SLsmg_fill_region(y, 1, 1, bar, ' ');
> >>>       SLsmg_refresh();
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>  }
> >>>
> >>>  static void tui_progress__finish(void)
> >>> @@ -67,12 +67,12 @@ static void tui_progress__finish(void)
> >>>               return;
> >>>
> >>>       ui__refresh_dimensions(false);
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>       y = SLtt_Screen_Rows / 2 - 2;
> >>>       SLsmg_set_color(0);
> >>>       SLsmg_fill_region(y, 0, 3, SLtt_Screen_Cols, ' ');
> >>>       SLsmg_refresh();
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>  }
> >>>
> >>>  static struct ui_progress_ops tui_progress__ops = {
> >>> diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> >>> index b1be59b4e2a4..a3b8c397c24d 100644
> >>> --- a/tools/perf/ui/tui/setup.c
> >>> +++ b/tools/perf/ui/tui/setup.c
> >>> @@ -29,10 +29,10 @@ void ui__refresh_dimensions(bool force)
> >>>  {
> >>>       if (force || ui__need_resize) {
> >>>               ui__need_resize = 0;
> >>> -             pthread_mutex_lock(&ui__lock);
> >>> +             mutex_lock(&ui__lock);
> >>>               SLtt_get_screen_size();
> >>>               SLsmg_reinit_smg();
> >>> -             pthread_mutex_unlock(&ui__lock);
> >>> +             mutex_unlock(&ui__lock);
> >>>       }
> >>>  }
> >>>
> >>> @@ -170,10 +170,10 @@ void ui__exit(bool wait_for_ok)
> >>>                                   "Press any key...", 0);
> >>>
> >>>       SLtt_set_cursor_visibility(1);
> >>> -     if (!pthread_mutex_trylock(&ui__lock)) {
> >>> +     if (mutex_trylock(&ui__lock)) {
> >>>               SLsmg_refresh();
> >>>               SLsmg_reset_smg();
> >>> -             pthread_mutex_unlock(&ui__lock);
> >>> +             mutex_unlock(&ui__lock);
> >>>       }
> >>>       SLang_reset_tty();
> >>>       perf_error__unregister(&perf_tui_eops);
> >>> diff --git a/tools/perf/ui/tui/util.c b/tools/perf/ui/tui/util.c
> >>> index 0f562e2cb1e8..3c5174854ac8 100644
> >>> --- a/tools/perf/ui/tui/util.c
> >>> +++ b/tools/perf/ui/tui/util.c
> >>> @@ -95,7 +95,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >>>               t = sep + 1;
> >>>       }
> >>>
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>
> >>>       max_len += 2;
> >>>       nr_lines += 8;
> >>> @@ -125,17 +125,17 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >>>       SLsmg_write_nstring((char *)exit_msg, max_len);
> >>>       SLsmg_refresh();
> >>>
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>
> >>>       x += 2;
> >>>       len = 0;
> >>>       key = ui__getch(delay_secs);
> >>>       while (key != K_TIMER && key != K_ENTER && key != K_ESC) {
> >>> -             pthread_mutex_lock(&ui__lock);
> >>> +             mutex_lock(&ui__lock);
> >>>
> >>>               if (key == K_BKSPC) {
> >>>                       if (len == 0) {
> >>> -                             pthread_mutex_unlock(&ui__lock);
> >>> +                             mutex_unlock(&ui__lock);
> >>>                               goto next_key;
> >>>                       }
> >>>                       SLsmg_gotorc(y, x + --len);
> >>> @@ -147,7 +147,7 @@ int ui_browser__input_window(const char *title, const char *text, char *input,
> >>>               }
> >>>               SLsmg_refresh();
> >>>
> >>> -             pthread_mutex_unlock(&ui__lock);
> >>> +             mutex_unlock(&ui__lock);
> >>>
> >>>               /* XXX more graceful overflow handling needed */
> >>>               if (len == sizeof(buf) - 1) {
> >>> @@ -215,19 +215,19 @@ void __ui__info_window(const char *title, const char *text, const char *exit_msg
> >>>
> >>>  void ui__info_window(const char *title, const char *text)
> >>>  {
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>       __ui__info_window(title, text, NULL);
> >>>       SLsmg_refresh();
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>  }
> >>>
> >>>  int ui__question_window(const char *title, const char *text,
> >>>                       const char *exit_msg, int delay_secs)
> >>>  {
> >>> -     pthread_mutex_lock(&ui__lock);
> >>> +     mutex_lock(&ui__lock);
> >>>       __ui__info_window(title, text, exit_msg);
> >>>       SLsmg_refresh();
> >>> -     pthread_mutex_unlock(&ui__lock);
> >>> +     mutex_unlock(&ui__lock);
> >>>       return ui__getch(delay_secs);
> >>>  }
> >>>
> >>> diff --git a/tools/perf/ui/ui.h b/tools/perf/ui/ui.h
> >>> index 9b6fdf06e1d2..99f8d2fe9bc5 100644
> >>> --- a/tools/perf/ui/ui.h
> >>> +++ b/tools/perf/ui/ui.h
> >>> @@ -2,11 +2,11 @@
> >>>  #ifndef _PERF_UI_H_
> >>>  #define _PERF_UI_H_ 1
> >>>
> >>> -#include <pthread.h>
> >>> +#include "../util/mutex.h"
> >>>  #include <stdbool.h>
> >>>  #include <linux/compiler.h>
> >>>
> >>> -extern pthread_mutex_t ui__lock;
> >>> +extern struct mutex ui__lock;
> >>>  extern void *perf_gtk_handle;
> >>>
> >>>  extern int use_browser;
> >>
>

Adrian Hunter Aug. 26, 2022, 6:52 p.m. UTC | #7

On 26/08/22 20:45, Ian Rogers wrote:
> On Fri, Aug 26, 2022 at 10:22 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 26/08/22 19:02, Ian Rogers wrote:
>>> On Fri, Aug 26, 2022 at 3:24 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> On 24/08/22 18:38, Ian Rogers wrote:
>>>>> Switch to the use of mutex wrappers that provide better error checking.
>>>>>
>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>>>> ---
>>>>>  tools/perf/ui/browser.c           | 20 ++++++++++----------
>>>>>  tools/perf/ui/browsers/annotate.c |  2 +-
>>>>>  tools/perf/ui/setup.c             |  5 +++--
>>>>>  tools/perf/ui/tui/helpline.c      |  5 ++---
>>>>>  tools/perf/ui/tui/progress.c      |  8 ++++----
>>>>>  tools/perf/ui/tui/setup.c         |  8 ++++----
>>>>>  tools/perf/ui/tui/util.c          | 18 +++++++++---------
>>>>>  tools/perf/ui/ui.h                |  4 ++--
>>>>>  8 files changed, 35 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>>>>> index fa5bd5c20e96..78fb01d6ad63 100644
>>>>> --- a/tools/perf/ui/browser.c
>>>>> +++ b/tools/perf/ui/browser.c
>>>>> @@ -268,9 +268,9 @@ void __ui_browser__show_title(struct ui_browser *browser, const char *title)
>>>>>
>>>>>  void ui_browser__show_title(struct ui_browser *browser, const char *title)
>>>>>  {
>>>>> -     pthread_mutex_lock(&ui__lock);
>>>>> +     mutex_lock(&ui__lock);
>>>>>       __ui_browser__show_title(browser, title);
>>>>> -     pthread_mutex_unlock(&ui__lock);
>>>>> +     mutex_unlock(&ui__lock);
>>>>>  }
>>>>>
>>>>>  int ui_browser__show(struct ui_browser *browser, const char *title,
>>>>> @@ -284,7 +284,7 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>>>>
>>>>>       browser->refresh_dimensions(browser);
>>>>>
>>>>> -     pthread_mutex_lock(&ui__lock);
>>>>> +     mutex_lock(&ui__lock);
>>>>>       __ui_browser__show_title(browser, title);
>>>>>
>>>>>       browser->title = title;
>>>>> @@ -295,16 +295,16 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>>>>>       va_end(ap);
>>>>>       if (err > 0)
>>>>>               ui_helpline__push(browser->helpline);
>>>>> -     pthread_mutex_unlock(&ui__lock);
>>>>> +     mutex_unlock(&ui__lock);
>>>>>       return err ? 0 : -1;
>>>>>  }
>>>>>
>>>>>  void ui_browser__hide(struct ui_browser *browser)
>>>>>  {
>>>>> -     pthread_mutex_lock(&ui__lock);
>>>>> +     mutex_lock(&ui__lock);
>>>>>       ui_helpline__pop();
>>>>>       zfree(&browser->helpline);
>>>>> -     pthread_mutex_unlock(&ui__lock);
>>>>> +     mutex_unlock(&ui__lock);
>>>>>  }
>>>>>
>>>>>  static void ui_browser__scrollbar_set(struct ui_browser *browser)
>>>>> @@ -352,9 +352,9 @@ static int __ui_browser__refresh(struct ui_browser *browser)
>>>>>
>>>>>  int ui_browser__refresh(struct ui_browser *browser)
>>>>>  {
>>>>> -     pthread_mutex_lock(&ui__lock);
>>>>> +     mutex_lock(&ui__lock);
>>>>>       __ui_browser__refresh(browser);
>>>>> -     pthread_mutex_unlock(&ui__lock);
>>>>> +     mutex_unlock(&ui__lock);
>>>>>
>>>>>       return 0;
>>>>>  }
>>>>> @@ -390,10 +390,10 @@ int ui_browser__run(struct ui_browser *browser, int delay_secs)
>>>>>       while (1) {
>>>>>               off_t offset;
>>>>>
>>>>> -             pthread_mutex_lock(&ui__lock);
>>>>> +             mutex_lock(&ui__lock);
>>>>>               err = __ui_browser__refresh(browser);
>>>>>               SLsmg_refresh();
>>>>> -             pthread_mutex_unlock(&ui__lock);
>>>>> +             mutex_unlock(&ui__lock);
>>>>>               if (err < 0)
>>>>>                       break;
>>>>>
>>>>> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
>>>>> index 44ba900828f6..b8747e8dd9ea 100644
>>>>> --- a/tools/perf/ui/browsers/annotate.c
>>>>> +++ b/tools/perf/ui/browsers/annotate.c
>>>>> @@ -8,11 +8,11 @@
>>>>>  #include "../../util/hist.h"
>>>>>  #include "../../util/sort.h"
>>>>>  #include "../../util/map.h"
>>>>> +#include "../../util/mutex.h"
>>>>>  #include "../../util/symbol.h"
>>>>>  #include "../../util/evsel.h"
>>>>>  #include "../../util/evlist.h"
>>>>>  #include <inttypes.h>
>>>>> -#include <pthread.h>
>>>>>  #include <linux/kernel.h>
>>>>>  #include <linux/string.h>
>>>>>  #include <linux/zalloc.h>
>>>>> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
>>>>> index 700335cde618..25ded88801a3 100644
>>>>> --- a/tools/perf/ui/setup.c
>>>>> +++ b/tools/perf/ui/setup.c
>>>>> @@ -1,5 +1,4 @@
>>>>>  // SPDX-License-Identifier: GPL-2.0
>>>>> -#include <pthread.h>
>>>>>  #include <dlfcn.h>
>>>>>  #include <unistd.h>
>>>>>
>>>>> @@ -8,7 +7,7 @@
>>>>>  #include "../util/hist.h"
>>>>>  #include "ui.h"
>>>>>
>>>>> -pthread_mutex_t ui__lock = PTHREAD_MUTEX_INITIALIZER;
>>>>> +struct mutex ui__lock;
>>>>>  void *perf_gtk_handle;
>>>>>  int use_browser = -1;
>>>>>
>>>>> @@ -76,6 +75,7 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>>>>>
>>>>>  void setup_browser(bool fallback_to_pager)
>>>>>  {
>>>>> +     mutex_init(&ui__lock);
>>>>>       if (use_browser < 2 && (!isatty(1) || dump_trace))
>>>>>               use_browser = 0;
>>>>>
>>>>> @@ -118,4 +118,5 @@ void exit_browser(bool wait_for_ok)
>>>>>       default:
>>>>>               break;
>>>>>       }
>>>>> +     mutex_destroy(&ui__lock);
>>>>
>>>> Looks like exit_browser() can be called even when setup_browser()
>>>> was never called.
>>>>
>>>> Note, it also looks like PTHREAD_MUTEX_INITIALIZER is all zeros so
>>>> pthread won't notice.
>>>
>>> Memory sanitizer will notice some cases of this and so I didn't want
>>> to code defensively around exit being called ahead of setup.
>>
>> I am not sure you understood.
>>
>> As I wrote, exit_browser() can be called even when setup_browser()
>> was never called, so it is not defensive programming, it is necessary
>> programming that you only get away without doing because
>> PTHREAD_MUTEX_INITIALIZER is all zeros.
> 
> Why are we here:
> 1) there is a memory leak
> 2) I fix the memory and trigger a use after free
> 3) I invent a reference count checker, use it to fix the memory leak,
> use after free and missing locks - the patch for this in 10s of lines
> long
> 4) when adding the lock fixes I defensively add error checking to the
> mutex involved - mainly because I was scared I could introduce a
> deadlock
> 5) I get asked to generalize this
> 6) GSoC contributor picks up and puts this down
> 7) I pull together the contributor's work
> 8) I get asked to turn a search and replace 4 patch fix into an unwieldy patch
> 9) I worry about the sanity of the change and add lock checking from clang
> 10) I end up trying to fix perf-sched who for some reason thought it
> was perfectly valid to have threads blocked on mutexes that were
> deallocated on the stack.
> 11) the UI code was written with a view that exiting something not
> setup somehow made sense
> 
> I am drawing a line at fixing perf sched and the UI code. We can drop
> this patch and keep things as a pthread_mutex_t, similarly for
> perf-sched. I have gone about as far as I'm prepared to for the sake
> of a 10s of line memory leak fix. Some private thoughts are, it would
> be useful if review comments could be constructive, hey do this not
> that, and not simply commenting on change or trying to shoehorn vast
> amounts of tech debt clean up onto simple fixes.

If you want help, you only need ask.

Below seems adequate for now, at least logically, but maybe it
would confuse clang thread-safety analysis?

diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
index 25ded88801a3..6d81be6a349e 100644
--- a/tools/perf/ui/setup.c
+++ b/tools/perf/ui/setup.c
@@ -73,9 +73,17 @@ int stdio__config_color(const struct option *opt __maybe_unused,
 	return 0;
 }
 
+/*
+ * exit_browser() can get called without setup_browser() having been called
+ * first, so it is necessary to keep track of whether ui__lock has been
+ * initialized.
+ */
+static bool ui__lock_initialized;
+
 void setup_browser(bool fallback_to_pager)
 {
 	mutex_init(&ui__lock);
+	ui__lock_initialized = true;
 	if (use_browser < 2 && (!isatty(1) || dump_trace))
 		use_browser = 0;
 
@@ -118,5 +126,6 @@ void exit_browser(bool wait_for_ok)
 	default:
 		break;
 	}
-	mutex_destroy(&ui__lock);
+	if (ui__lock_initialized)
+		mutex_destroy(&ui__lock);
 }

Namhyung Kim Aug. 26, 2022, 7 p.m. UTC | #8

On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> Below seems adequate for now, at least logically, but maybe it
> would confuse clang thread-safety analysis?

I think it's not just about locks, the exit_browser should bail out early
if the setup code was not called.

Thanks,
Namhyung


>
> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
> index 25ded88801a3..6d81be6a349e 100644
> --- a/tools/perf/ui/setup.c
> +++ b/tools/perf/ui/setup.c
> @@ -73,9 +73,17 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>         return 0;
>  }
>
> +/*
> + * exit_browser() can get called without setup_browser() having been called
> + * first, so it is necessary to keep track of whether ui__lock has been
> + * initialized.
> + */
> +static bool ui__lock_initialized;
> +
>  void setup_browser(bool fallback_to_pager)
>  {
>         mutex_init(&ui__lock);
> +       ui__lock_initialized = true;
>         if (use_browser < 2 && (!isatty(1) || dump_trace))
>                 use_browser = 0;
>
> @@ -118,5 +126,6 @@ void exit_browser(bool wait_for_ok)
>         default:
>                 break;
>         }
> -       mutex_destroy(&ui__lock);
> +       if (ui__lock_initialized)
> +               mutex_destroy(&ui__lock);
>  }
>

Adrian Hunter Aug. 26, 2022, 7:20 p.m. UTC | #9

On 26/08/22 22:00, Namhyung Kim wrote:
> On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Below seems adequate for now, at least logically, but maybe it
>> would confuse clang thread-safety analysis?
> 
> I think it's not just about locks, the exit_browser should bail out early
> if the setup code was not called.

In those cases, use_browser is 0 or -1 unless someone has inserted
an invalid perf config like "tui.script=on" or "gtk.script=on".
So currently, in cases where exit_browser() is called without
setup_browser(), it does nothing.  Which means it is only the
unconditional mutex_destroy() that needs to be conditional.

> 
> Thanks,
> Namhyung
> 
> 
>>
>> diff --git a/tools/perf/ui/setup.c b/tools/perf/ui/setup.c
>> index 25ded88801a3..6d81be6a349e 100644
>> --- a/tools/perf/ui/setup.c
>> +++ b/tools/perf/ui/setup.c
>> @@ -73,9 +73,17 @@ int stdio__config_color(const struct option *opt __maybe_unused,
>>         return 0;
>>  }
>>
>> +/*
>> + * exit_browser() can get called without setup_browser() having been called
>> + * first, so it is necessary to keep track of whether ui__lock has been
>> + * initialized.
>> + */
>> +static bool ui__lock_initialized;
>> +
>>  void setup_browser(bool fallback_to_pager)
>>  {
>>         mutex_init(&ui__lock);
>> +       ui__lock_initialized = true;
>>         if (use_browser < 2 && (!isatty(1) || dump_trace))
>>                 use_browser = 0;
>>
>> @@ -118,5 +126,6 @@ void exit_browser(bool wait_for_ok)
>>         default:
>>                 break;
>>         }
>> -       mutex_destroy(&ui__lock);
>> +       if (ui__lock_initialized)
>> +               mutex_destroy(&ui__lock);
>>  }
>>

Namhyung Kim Aug. 26, 2022, 8:40 p.m. UTC | #10

On Fri, Aug 26, 2022 at 12:21 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 26/08/22 22:00, Namhyung Kim wrote:
> > On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >> Below seems adequate for now, at least logically, but maybe it
> >> would confuse clang thread-safety analysis?
> >
> > I think it's not just about locks, the exit_browser should bail out early
> > if the setup code was not called.
>
> In those cases, use_browser is 0 or -1 unless someone has inserted
> an invalid perf config like "tui.script=on" or "gtk.script=on".
> So currently, in cases where exit_browser() is called without
> setup_browser(), it does nothing.  Which means it is only the
> unconditional mutex_destroy() that needs to be conditional.

Yeah there's a possibility that it can be called with > 0 use_browser
on some broken config or something.  So I think it's safer and better
for future changes.

Thanks,
Namhyung

Ian Rogers Aug. 26, 2022, 8:52 p.m. UTC | #11

On Fri, Aug 26, 2022 at 1:40 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, Aug 26, 2022 at 12:21 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > On 26/08/22 22:00, Namhyung Kim wrote:
> > > On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> > >> Below seems adequate for now, at least logically, but maybe it
> > >> would confuse clang thread-safety analysis?
> > >
> > > I think it's not just about locks, the exit_browser should bail out early
> > > if the setup code was not called.
> >
> > In those cases, use_browser is 0 or -1 unless someone has inserted
> > an invalid perf config like "tui.script=on" or "gtk.script=on".
> > So currently, in cases where exit_browser() is called without
> > setup_browser(), it does nothing.  Which means it is only the
> > unconditional mutex_destroy() that needs to be conditional.
>
> Yeah there's a possibility that it can be called with > 0 use_browser
> on some broken config or something.  So I think it's safer and better
> for future changes.

I'd thought about a:
static bool ui__lock_initialized;
but the issue is shouldn't it be atomic? Maybe we should guard it with
a lock? Then we are back where we started. Having a clean init/exit
invariant would be best but such a change has the potential to be
large and out of scope here.

Thanks,
Ian

> Thanks,
> Namhyung

Adrian Hunter Aug. 27, 2022, 7:11 a.m. UTC | #12

On 26/08/22 23:52, Ian Rogers wrote:
> On Fri, Aug 26, 2022 at 1:40 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>
>> On Fri, Aug 26, 2022 at 12:21 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 26/08/22 22:00, Namhyung Kim wrote:
>>>> On Fri, Aug 26, 2022 at 11:53 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> Below seems adequate for now, at least logically, but maybe it
>>>>> would confuse clang thread-safety analysis?
>>>>
>>>> I think it's not just about locks, the exit_browser should bail out early
>>>> if the setup code was not called.
>>>
>>> In those cases, use_browser is 0 or -1 unless someone has inserted
>>> an invalid perf config like "tui.script=on" or "gtk.script=on".
>>> So currently, in cases where exit_browser() is called without
>>> setup_browser(), it does nothing.  Which means it is only the
>>> unconditional mutex_destroy() that needs to be conditional.
>>
>> Yeah there's a possibility that it can be called with > 0 use_browser
>> on some broken config or something.  So I think it's safer and better
>> for future changes.
> 
> I'd thought about a:
> static bool ui__lock_initialized;
> but the issue is shouldn't it be atomic?

No, it is only accessed from the main thread.

[v3,09/18] perf ui: Update use of pthread mutex

Checks

Commit Message

Comments

Patch