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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 15 Apr 2016 13:26:54 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:	"Wangnan (F)" <wangnan0@...wei.com>, Jiri Olsa <jolsa@...hat.com>,
	linux-kernel@...r.kernel.org, pi3orama@....com,
	He Kuang <hekuang@...wei.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Zefan Li <lizefan@...wei.com>
Subject: Re: [PATCH 00/10] perf tools: Use SIGUSR2 control data dumpping

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