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

["Wangnan (F)" <wangnan0@huawei.com>]

On 2016/4/14 15:15, Adrian Hunter wrote:
> 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?

This patch doesn't prevent taking snapshot when receiving SIGUSR2.

If both --snapshot and --switch-outupt is provided, when SIGUSR2 received,
perf takes auxtrace snapshot and other perf buffer together.

Thank you.


> 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