lkml.org 
[lkml]   [2016]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping
    From
    Date


    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
    >


    \
     
     \ /
      Last update: 2016-04-15 20:01    [W:3.308 / U:0.728 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site