Messages in this thread | | | Subject | Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping | From | "Wangnan (F)" <> | Date | Sat, 16 Apr 2016 01:56:28 +0800 |
| |
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 >
| |