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

[Wangnan (F)]

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