Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping

From	"Wangnan (F)" <wangnan0@huawei.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping
Date	2016-04-15 20:00 +0200
Message-ID	<rogca-6Qh-15@gated-at.bofh.it> (permalink)
References	(2 earlier) <ro9u2-1AH-17@gated-at.bofh.it> <roaq6-2jC-9@gated-at.bofh.it> <robFv-3uA-9@gated-at.bofh.it> <roeN4-5WB-9@gated-at.bofh.it> <rof6q-63Q-23@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw


On 2016/4/16 0:48, Wangnan (F) wrote:
>
>
> On 2016/4/16 0:26, Arnaldo Carvalho de Melo wrote:
>> Em Fri, Apr 15, 2016 at 10:09:32AM -0300, Arnaldo Carvalho de Melo 
>> escreveu:
>>> Em Fri, Apr 15, 2016 at 07:40:44PM +0800, Wangnan (F) escreveu:
>>>> On 2016/4/15 18:45, Wangnan (F) wrote:
>>>>> On 2016/4/15 18:40, Jiri Olsa wrote:
>>>>>> On Wed, Apr 13, 2016 at 08:21:03AM +0000, Wang Nan wrote:
>>>> [SNIP]
>>>>
>>>>>> I did not get 3/10 patch and the patchset did not apply cleanly,
>>>>>> git am failed.. would you have it in a branch somewhere?
>>>>> Sorry, you are not in the CC list. 'git send-email' failed to 
>>>>> extract your
>>>>> email address from the Acked-by tag.
>>>>>
>>>>> I'll inform you after I putting them into a git branch. Please wait.
>>>>>
>>>>> Thank you.
>>>> I just realized Arnaldo has already collected these patches set into
>>>> his perf/core. Please see it in his git tree [1]. You can also have 
>>>> a look
>>>> at my git tree [2] if you want :)
>>> I haven't pushed them to Ingo yet, so I can fix up things if Jiri has
>>> any fixes or other requests,
>> I moved those patches to a separate branch, perf/switch_output, till 
>> we get a
>> bit more review, I think I was too fast on tentatively processing 
>> this patchset
>> and have some questions, for instance, this part I thin really 
>> confusing, in
>> the main record loop:
>>
>>             switch_output_enable();
>>          for (;;) {
>>                  unsigned long long hits = rec->samples;
>>
>>                  if (record__mmap_read_all(rec) < 0) {
>>                          auxtrace_snapshot_disable();
>>                          err = -1;
>>                          goto out_child;
>>                  }
>> <SNIP>
>>                  if (switch_output_is_disabled()) {
>>                          switch_output_enable();
>>
>>                          if (!quiet)
>>                                  fprintf(stderr, "[ perf record: dump 
>> data: Woken up %ld times ]\n",
>>                                          waking);
>>                          waking = 0;
>>                          fd = record__switch_output(rec, false);
>>                          if (fd < 0) {
>>                                  pr_err("Failed to switch to new 
>> file\n");
>>                                  err = fd;
>>                                  goto out_child;
>>                          }
>>                  }
>> <SNIP>
>>     }
>>
>> That switch_output_enable() one we can't get to because it is part of 
>> that
>> trigger_ thing, so just by looking here we think switch_output is 
>> being enabled
>> unconditionally, when in fact it will check if it is "OFF" and if so, 
>> will not
>> "enable", then when we see switch_output_is_disabled() the question 
>> will return
>> false if it is "OFF", but what we read is "hey, this is not disabled, 
>> so it
>> must be enabled, no? Confusing :-\
>
> You are right. I think we should change the naming in trigger stuff:
>
> TRIGGER_OFF:       this trigger is turned off
> TRIGGER_RELEASED:   preparing to be triggered
> TRIGGER_TOGGLED: things happened
>
> actions:
>
> OFF -> RELEASED    : on
> RELEASED -> TOGGLED: toggle
> TOGGLED-> RELEASED : release
>
> conditions:
>
> is_released()
> is_toggled()
>
> I'll send a v3 soon.
>
> Thank you.
>
>> Perhaps we should have multiple record loops, one really simple, the 
>> original
>> one, one for auxtrace, that, from what we've discussed so far, 
>> doesn't lok will
>> be supported together with output switch, and one for output switch?
>>
>> Would be something like:
>>
>>     if (switch_output)
>>         err = record__switch_output_read_events()
>>     else if (auxtrace)
>>         err = record__auxtrace_read_events()
>>     else
>>         err = record__read_events();
>>
>> And then each of these loops don't need to have all those branches 
>> per mmap_read.
>>

Auxtrace and original events are not exclusive. Auxtrace and 
switch_output are
not necessarily exclusive. At lease intel_bts// works fine. It is 
intel_pt's own
limitation, not all auxtrace events.

Thank you.

>> - Arnaldo
>>> - Arnaldo
>>>> [1] 
>>>> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=c0bdc1c461dd5b2e492c0746708a3e0da6b13515
>>>> [2] 
>>>> https://git.kernel.org/cgit/linux/kernel/git/pi3orama/linux.git/log/?h=perf/overwrite
>

Thread

[PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping Wang Nan <wangnan0@huawei.com> - 2016-04-13 10:30 +0200
  [PATCH 08/10] perf record: Re-synthesize tracking events after output switching Wang Nan <wangnan0@huawei.com> - 2016-04-13 10:30 +0200
  [PATCH 04/10] perf record: Add '--timestamp-filename' option to append timestamp to output filename Wang Nan <wangnan0@huawei.com> - 2016-04-13 10:30 +0200
    [tip:perf/core] perf record: Add '--timestamp-filename' option to  append timestamp to output file name tip-bot for Wang Nan <tipbot@zytor.com> - 2016-04-14 15:40 +0200
  [PATCH 10/10] perf core: Add backward attribute to perf event Wang Nan <wangnan0@huawei.com> - 2016-04-13 10:30 +0200
  [PATCH 02/10] perf tools: Add perf_data_file__switch() helper Wang Nan <wangnan0@huawei.com> - 2016-04-13 10:30 +0200
    [tip:perf/core] perf data: Add perf_data_file__switch() helper tip-bot for Wang Nan <tipbot@zytor.com> - 2016-04-14 15:40 +0200
    Re: [PATCH 02/10] perf tools: Add perf_data_file__switch() helper Jiri Olsa <jolsa@redhat.com> - 2016-04-15 13:00 +0200
      Re: [PATCH 02/10] perf tools: Add perf_data_file__switch() helper Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-04-15 18:10 +0200
  [PATCH 09/10] perf record: Generate tracking events for process forked by perf Wang Nan <wangnan0@huawei.com> - 2016-04-13 10:30 +0200
  [PATCH 05/10] perf record: Split output into multiple files via '--switch-output' Wang Nan <wangnan0@huawei.com> - 2016-04-13 10:30 +0200
  Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-04-13 19:20 +0200
  Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping "Wangnan (F)" <wangnan0@huawei.com> - 2016-04-15 12:50 +0200
    Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping "Wangnan (F)" <wangnan0@huawei.com> - 2016-04-15 13:50 +0200
      Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-04-15 15:10 +0200
        Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-04-15 18:30 +0200
          Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping "Wangnan (F)" <wangnan0@huawei.com> - 2016-04-15 18:50 +0200
            Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping "Wangnan (F)" <wangnan0@huawei.com> - 2016-04-15 20:00 +0200
  Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping Jiri Olsa <jolsa@redhat.com> - 2016-04-15 12:50 +0200

csiph-web