Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3 states

[Adrian Hunter <adrian.hunter@intel.com>]

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@huawei.com>
>> Signed-off-by: He Kuang <hekuang@huawei.com>
>> Acked-by: Jiri Olsa <jolsa@kernel.org>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Zefan Li <lizefan@huawei.com>
>> Cc: pi3orama@163.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
>