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

From: Wangnan (F)
Date: Fri Apr 15 2016 - 12:49:34 EST




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.

- 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