Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1377635 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3 states Wang Nan <wangnan0@huawei.com> - 2016-04-13 10:30 +0200
    Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3  states Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-04-13 18:00 +0200
      Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3  states Adrian Hunter <adrian.hunter@intel.com> - 2016-04-14 09:20 +0200
        Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3  states "Wangnan (F)" <wangnan0@huawei.com> - 2016-04-14 10:00 +0200
          Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3  states Adrian Hunter <adrian.hunter@intel.com> - 2016-04-14 10:40 +0200
            Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3  states "Wangnan (F)" <wangnan0@huawei.com> - 2016-04-14 11:10 +0200
              Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot_enable into 3  states Adrian Hunter <adrian.hunter@intel.com> - 2016-04-14 12:30 +0200
    [tip:perf/core] perf record: Turns auxtrace_snapshot_enable into 3  states tip-bot for Wang Nan <tipbot@zytor.com> - 2016-04-14 15:40 +0200

#1377635 — [PATCH 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(-)

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

[toc] | [next] | [standalone]

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

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?

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

[toc] | [prev] | [next] | [standalone]

#1378541 — 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@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
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

On 14/04/16 12:07, Wangnan (F) wrote:
> 
> 
> 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.

OK, I see.

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

Thanks for the information.  Auxtrace really needs complete MMAP
information, so I would probably need to look at other options as well.

Anyway for this patch:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

[toc] | [prev] | [next] | [standalone]

#1378862 — [tip:perf/core] perf record: Turns auxtrace_snapshot_enable into 3 states

Commit-ID:  c0bdc1c461dd5b2e492c0746708a3e0da6b13515
Gitweb:     http://git.kernel.org/tip/c0bdc1c461dd5b2e492c0746708a3e0da6b13515
Author:     Wang Nan <wangnan0@huawei.com>
AuthorDate: Wed, 13 Apr 2016 08:21:06 +0000
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 14 Apr 2016 08:58:19 -0300

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>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Zefan Li <lizefan@huawei.com>
Cc: pi3orama@163.com
Link: http://lkml.kernel.org/r/1460535673-159866-4-git-send-email-wangnan0@huawei.com
Signed-off-by: He Kuang <hekuang@huawei.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.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;
 }

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web