linux-kernel - Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 14 Apr 2016 10:15:31 +0300
From:	Adrian Hunter <adrian.hunter@...el.com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	Wang Nan <wangnan0@...wei.com>, linux-kernel@...r.kernel.org,
	pi3orama@....com, He Kuang <hekuang@...wei.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Zefan Li <lizefan@...wei.com>
Subject: Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3
 states

On 13/04/16 18:55, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 13, 2016 at 08:21:06AM +0000, Wang Nan escreveu:
>> auxtrace_snapshot_enable has only two states (0/1). Turns it into a
>> triple states enum so SIGUSR2 handler can safely do other works without
>> triggering auxtrace snapshot.
> 
> Adrian, can you take a look at this? Is it ok with you?

Please forgive me if these are stupid questions:

First I am wondering why we wouldn't want to snapshot auxtrace data at the
same time as the perf buffer?

For that we would need continuous tracking information like MMAPS etc, but
isn't that needed anyway?


> 
> Thanks,
> 
> - Arnaldo
>  
>> Signed-off-by: Wang Nan <wangnan0@...wei.com>
>> Signed-off-by: He Kuang <hekuang@...wei.com>
>> Acked-by: Jiri Olsa <jolsa@...nel.org>
>> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
>> Cc: Namhyung Kim <namhyung@...nel.org>
>> Cc: Zefan Li <lizefan@...wei.com>
>> Cc: pi3orama@....com
>> ---
>>  tools/perf/builtin-record.c | 59 +++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 49 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index eb6a199..480033f 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -125,7 +125,43 @@ out:
>>  static volatile int done;
>>  static volatile int signr = -1;
>>  static volatile int child_finished;
>> -static volatile int auxtrace_snapshot_enabled;
>> +
>> +static volatile enum {
>> +	AUXTRACE_SNAPSHOT_OFF = -1,
>> +	AUXTRACE_SNAPSHOT_DISABLED = 0,
>> +	AUXTRACE_SNAPSHOT_ENABLED = 1,
>> +} auxtrace_snapshot_state = AUXTRACE_SNAPSHOT_OFF;
>> +
>> +static inline void
>> +auxtrace_snapshot_on(void)
>> +{
>> +	auxtrace_snapshot_state = AUXTRACE_SNAPSHOT_DISABLED;
>> +}
>> +
>> +static inline void
>> +auxtrace_snapshot_enable(void)
>> +{
>> +	if (auxtrace_snapshot_state == AUXTRACE_SNAPSHOT_OFF)
>> +		return;
>> +	auxtrace_snapshot_state = AUXTRACE_SNAPSHOT_ENABLED;
>> +}
>> +
>> +static inline void
>> +auxtrace_snapshot_disable(void)
>> +{
>> +	if (auxtrace_snapshot_state == AUXTRACE_SNAPSHOT_OFF)
>> +		return;
>> +	auxtrace_snapshot_state = AUXTRACE_SNAPSHOT_DISABLED;
>> +}
>> +
>> +static inline bool
>> +auxtrace_snapshot_is_enabled(void)
>> +{
>> +	if (auxtrace_snapshot_state == AUXTRACE_SNAPSHOT_OFF)
>> +		return false;
>> +	return auxtrace_snapshot_state == AUXTRACE_SNAPSHOT_ENABLED;
>> +}
>> +
>>  static volatile int auxtrace_snapshot_err;
>>  static volatile int auxtrace_record__snapshot_started;
>>  
>> @@ -249,7 +285,7 @@ static void record__read_auxtrace_snapshot(struct record *rec)
>>  	} else {
>>  		auxtrace_snapshot_err = auxtrace_record__snapshot_finish(rec->itr);
>>  		if (!auxtrace_snapshot_err)
>> -			auxtrace_snapshot_enabled = 1;
>> +			auxtrace_snapshot_enable();
>>  	}
>>  }
>>  
>> @@ -615,10 +651,13 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>>  	signal(SIGCHLD, sig_handler);
>>  	signal(SIGINT, sig_handler);
>>  	signal(SIGTERM, sig_handler);
>> -	if (rec->opts.auxtrace_snapshot_mode)
>> +
>> +	if (rec->opts.auxtrace_snapshot_mode) {
>>  		signal(SIGUSR2, snapshot_sig_handler);
>> -	else
>> +		auxtrace_snapshot_on();
>> +	} else {
>>  		signal(SIGUSR2, SIG_IGN);
>> +	}
>>  
>>  	session = perf_session__new(file, false, tool);
>>  	if (session == NULL) {
>> @@ -744,12 +783,12 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>>  		perf_evlist__enable(rec->evlist);
>>  	}
>>  
>> -	auxtrace_snapshot_enabled = 1;
>> +	auxtrace_snapshot_enable();
>>  	for (;;) {
>>  		unsigned long long hits = rec->samples;
>>  
>>  		if (record__mmap_read_all(rec) < 0) {
>> -			auxtrace_snapshot_enabled = 0;
>> +			auxtrace_snapshot_disable();
>>  			err = -1;
>>  			goto out_child;
>>  		}
>> @@ -787,12 +826,12 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>>  		 * disable events in this case.
>>  		 */
>>  		if (done && !disabled && !target__none(&opts->target)) {
>> -			auxtrace_snapshot_enabled = 0;
>> +			auxtrace_snapshot_disable();
>>  			perf_evlist__disable(rec->evlist);
>>  			disabled = true;
>>  		}
>>  	}
>> -	auxtrace_snapshot_enabled = 0;
>> +	auxtrace_snapshot_disable();
>>  
>>  	if (forks && workload_exec_errno) {
>>  		char msg[STRERR_BUFSIZE];
>> @@ -1358,9 +1397,9 @@ out_symbol_exit:
>>  
>>  static void snapshot_sig_handler(int sig __maybe_unused)
>>  {
>> -	if (!auxtrace_snapshot_enabled)
>> +	if (!auxtrace_snapshot_is_enabled())
>>  		return;
>> -	auxtrace_snapshot_enabled = 0;
>> +	auxtrace_snapshot_disable();
>>  	auxtrace_snapshot_err = auxtrace_record__snapshot_start(record.itr);
>>  	auxtrace_record__snapshot_started = 1;
>>  }
>> -- 
>> 1.8.3.4
>