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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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 :-\

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