linux-kernel - Re: [PATCH v2 1/2] perf tui: don't ignore job control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZZch2_ceYO-UI71B@kernel.org>
Date: Thu, 4 Jan 2024 18:23:39 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ahelenia Ziemiańska <nabijaczleweli@...ijaczleweli.xyz>
Cc: Namhyung Kim <namhyung@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>, yaowenbin <yaowenbin1@...wei.com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] perf tui: don't ignore job control

Em Tue, Dec 27, 2022 at 09:57:40PM +0100, Ahelenia Ziemiańska escreveu:
> In its infinite wisdom, by default, SLang sets susp undef,
> and this can only be un-done by calling SLtty_set_suspend_state(true).
> After every SLang_init_tty().
> 
> Additionally, no provisions are made for maintaining the teletype
> attributes across suspend/continue (outside of curses emulation mode(?!),
> which provides full support, naturally), so we need to save and restore
> the flags ourselves, as well as reset the text colours when going under.
> We need to also re-draw the screen, and raising SIGWINCH, shockingly,
> Just Works.
> 
> The correct solution would be to Not Use SLang, but as a stop-gap,
> this makes TUI perf report usable.

This was in the compost pile for a long time, but I got to it, indeed
'perf report' works, the refresh after returning to it, etc.

'perf top' needs some care, but we can do it later.

About not using slang, perf started with newt, I moved to slang, there
are people suggesting we move to some more modern thing, maybe
notcurses?

- Arnaldo
 
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@...ijaczleweli.xyz>
> ---
> Yeah, somehow I didn't hit that in my tests, but it was relatively easy
> to trigger when I tried. A full partial write retry loop is an overkill
> here, I think, and few other places in perf actually try to resubmit
> partial writes, so.
> 
>  tools/perf/ui/browsers/annotate.c |  1 +
>  tools/perf/ui/browsers/hists.c    |  2 ++
>  tools/perf/ui/browsers/scripts.c  |  1 +
>  tools/perf/ui/tui/setup.c         | 22 ++++++++++++++++++++++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index c03fa76c02ff..6a4ffbf66c7f 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -942,6 +942,7 @@ int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
>  	/* reset abort key so that it can get Ctrl-C as a key */
>  	SLang_reset_tty();
>  	SLang_init_tty(0, 0, 0);
> +	SLtty_set_suspend_state(true);
>  
>  	return map_symbol__tui_annotate(&he->ms, evsel, hbt, opts);
>  }
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index b72ee6822222..2479e6d42e7c 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3010,6 +3010,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  	/* reset abort key so that it can get Ctrl-C as a key */
>  	SLang_reset_tty();
>  	SLang_init_tty(0, 0, 0);
> +	SLtty_set_suspend_state(true);
>  
>  	if (min_pcnt)
>  		browser->min_pcnt = min_pcnt;
> @@ -3682,6 +3683,7 @@ int block_hists_tui_browse(struct block_hist *bh, struct evsel *evsel,
>  	/* reset abort key so that it can get Ctrl-C as a key */
>  	SLang_reset_tty();
>  	SLang_init_tty(0, 0, 0);
> +	SLtty_set_suspend_state(true);
>  
>  	memset(&action, 0, sizeof(action));
>  
> diff --git a/tools/perf/ui/browsers/scripts.c b/tools/perf/ui/browsers/scripts.c
> index 47d2c7a8cbe1..50d45054ed6c 100644
> --- a/tools/perf/ui/browsers/scripts.c
> +++ b/tools/perf/ui/browsers/scripts.c
> @@ -166,6 +166,7 @@ void run_script(char *cmd)
>  	printf("\033[c\033[H\033[J");
>  	fflush(stdout);
>  	SLang_init_tty(0, 0, 0);
> +	SLtty_set_suspend_state(true);
>  	SLsmg_refresh();
>  }
>  
> diff --git a/tools/perf/ui/tui/setup.c b/tools/perf/ui/tui/setup.c
> index a3b8c397c24d..09524ba04975 100644
> --- a/tools/perf/ui/tui/setup.c
> +++ b/tools/perf/ui/tui/setup.c
> @@ -2,12 +2,14 @@
>  #include <signal.h>
>  #include <stdbool.h>
>  #include <stdlib.h>
> +#include <termios.h>
>  #include <unistd.h>
>  #include <linux/kernel.h>
>  #ifdef HAVE_BACKTRACE_SUPPORT
>  #include <execinfo.h>
>  #endif
>  
> +#include "../../util/color.h"
>  #include "../../util/debug.h"
>  #include "../../perf.h"
>  #include "../browser.h"
> @@ -122,6 +124,23 @@ static void ui__signal(int sig)
>  	exit(0);
>  }
>  
> +static void ui__sigcont(int sig)
> +{
> +	static struct termios tty;
> +
> +	if (sig == SIGTSTP) {
> +		while (tcgetattr(SLang_TT_Read_FD, &tty) == -1 && errno == EINTR)
> +			;
> +		while (write(SLang_TT_Read_FD, PERF_COLOR_RESET, sizeof(PERF_COLOR_RESET) - 1) == -1 && errno == EINTR)
> +			;
> +		raise(SIGSTOP);
> +	} else {
> +		while (tcsetattr(SLang_TT_Read_FD, TCSADRAIN, &tty) == -1 && errno == EINTR)
> +			;
> +		raise(SIGWINCH);
> +	}
> +}
> +
>  int ui__init(void)
>  {
>  	int err;
> @@ -136,6 +155,7 @@ int ui__init(void)
>  	err = SLang_init_tty(-1, 0, 0);
>  	if (err < 0)
>  		goto out;
> +	SLtty_set_suspend_state(true);
>  
>  	err = SLkp_init();
>  	if (err < 0) {
> @@ -150,6 +170,8 @@ int ui__init(void)
>  	signal(SIGINT, ui__signal);
>  	signal(SIGQUIT, ui__signal);
>  	signal(SIGTERM, ui__signal);
> +	signal(SIGTSTP, ui__sigcont);
> +	signal(SIGCONT, ui__sigcont);
>  
>  	perf_error__register(&perf_tui_eops);
>  
> -- 
> 2.30.2
> 



-- 

- Arnaldo