linux-kernel - Re: [PATCH 03/10] perf record: Turns auxtrace_snapshot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <570F5541.50407@intel.com>
Date:	Thu, 14 Apr 2016 11:30:57 +0300
From:	Adrian Hunter <adrian.hunter@...el.com>
To:	"Wangnan (F)" <wangnan0@...wei.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	linux-kernel@...r.kernel.org, pi3orama@....com,
	He Kuang <hekuang@...wei.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Zefan Li <lizefan@...wei.com>
Subject: 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@...wei.com>
>>>> Signed-off-by: He Kuang <hekuang@...wei.com>
>>>> Acked-by: Jiri Olsa <jolsa@...nel.org>
>>>> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
>>>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
>>>> Cc: Namhyung Kim <namhyung@...nel.org>
>>>> Cc: Zefan Li <lizefan@...wei.com>
>>>> Cc: pi3orama@....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
> 
> 
>