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

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.

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(-)

-- 
1.8.3.4

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

On 2016/4/14 16:30, Adrian Hunter wrote:
> On 14/04/16 10:50, Wangnan (F) wrote:

>>

>> 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.

> So it was a stupid question ;-)


Still thank you for pointing this. I suddenly realized the 
'switch_output_started'
in patch 5/10 is also need to be turned to a 3 state enum. If not, a 
SIGUSR2 incorrectly
triggers output switching even '--switch-output' is not provided when 
'--snapshot' exist.

>

>> If both --snapshot and --switch-outupt is provided, when SIGUSR2 received,

>> perf takes auxtrace snapshot and other perf buffer together.

> How do you keep from losing tracking information like MMAP events?  Are they

> is a different buffer?


Please see patch 8/10 and 9/10. MMAP events are resynthesized each time
when output file switched, so at the *head* of each 'perf.data' you can find
many MMAP/COMM/FORK... events.

After overwritable ring buffer is supported, there is a more aggresive
patch [1] resynthesize tracking events and put them at the *end* of
perf.data.

[1] 
https://git.kernel.org/cgit/linux/kernel/git/pi3orama/linux.git/commit/?h=perf/overwrite&id=747e10300397b9c28b01bca5bfad943c8cf2dcce