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

[Adrian Hunter]

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@xxxxxxxxxx>
>>>> Signed-off-by: He Kuang <hekuang@xxxxxxxxxx>
>>>> Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
>>>> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
>>>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
>>>> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
>>>> Cc: Zefan Li <lizefan@xxxxxxxxxx>
>>>> Cc: pi3orama@xxxxxxx
>>>> ---
>>>>   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
> 
> 
>