linux-kernel - RE: [PATCH 02/10] perf tool: fix: Don't discard prev in backward mode

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077537D1DEB@SHSMSX103.ccr.corp.intel.com>
Date:   Wed, 11 Oct 2017 02:12:21 +0000
From:   "Liang, Kan" <kan.liang@...el.com>
To:     "Wangnan (F)" <wangnan0@...wei.com>,
        "acme@...nel.org" <acme@...nel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "jolsa@...nel.org" <jolsa@...nel.org>,
        "hekuang@...wei.com" <hekuang@...wei.com>,
        "namhyung@...nel.org" <namhyung@...nel.org>,
        "alexander.shishkin@...ux.intel.com" 
        <alexander.shishkin@...ux.intel.com>,
        "Hunter, Adrian" <adrian.hunter@...el.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>
Subject: RE: [PATCH 02/10] perf tool: fix: Don't discard prev in backward
 mode

> >>
> >>> If you really want to avoid record duplication, you need to changes
> >>> record__mmap_read()'s
> >>> logic. Now it complains "failed to keep up with mmap data" and avoid
> >>> dumping data when
> >>> size of newly generated data is larger than the size of the ring
> >>> buffer. It is reasonable
> >>> for forward ring buffer because in this case you lost the head of the
> >>> first record, the
> >>> whole ring buffer is unparseable. However, it is wrong in backward
> >>> case. What you
> >>> should do in this case is dumping the whole ring buffer.
> >>>
> >> I think what you want should be something like this: (not tested)
> >>
> > No. That's not what I want.
> > My test code never trigger the WARN_ONCE.
> 
> The existing code never trigger that warning because the size computed
> by rb_find_range is never larger than size of ring buffer. After applying
> your patch, I believe it will trigger this WARN_ONCE and drop the whole
> ring buffer. Please set a smaller ring buffer and try again.
> 
> > I think you will see the problem, if you simply run the command as below.
> > sudo ./perf record -e cycles:P -C0 --overwrite --switch-output=1s
> >
> > The output size keep increasing. Because the new output always include
> the old outputs.
> > What I want is the 'start' and 'end' for the increase, not everything.
> 
> 
> This is my test result: add a '-m 1' for 'perf record' for shrinking
> ring buffer,
> start a while loop on CPU 0 to increase data rate.
> 
> It stops increasing after the ring buffer is full:
> 
> $:~/linux/tools/perf$ sudo ./perf record -m1 -e cycles:P -C0 --overwrite
> --switch-output=1s
>    Warning: File /home/w00229757/.perfconfig not owned by current user
> or root, ignoring it.
> [ perf record: dump data: Woken up 1 times ]
> [ perf record: Dump perf.data.2017101212165072 ]
> [ perf record: dump data: Woken up 1 times ]
> [ perf record: Dump perf.data.2017101212165175 ]
> [ perf record: dump data: Woken up 1 times ]
> [ perf record: Dump perf.data.2017101212165278 ]
> [ perf record: dump data: Woken up 1 times ]
> [ perf record: Dump perf.data.2017101212165381 ]
> [ perf record: dump data: Woken up 1 times ]
> [ perf record: Dump perf.data.2017101212165484 ]
> [ perf record: dump data: Woken up 1 times ]
> [ perf record: Dump perf.data.2017101212165586 ]
> ^C[ perf record: Woken up 1 times to write data ]
> [ perf record: Dump perf.data.2017101212165653 ]
> [ perf record: Captured and wrote 1.013 MB perf.data.<timestamp> ]
> 
> $ ls -l ./perf.data*
> -rw------- 1 root root  538988 Oct 12 12:16 ./perf.data.2017101212165072
> -rw------- 1 root root  538988 Oct 12 12:16 ./perf.data.2017101212165175
> -rw------- 1 root root  538988 Oct 12 12:16 ./perf.data.2017101212165278
> -rw------- 1 root root  538988 Oct 12 12:16 ./perf.data.2017101212165381
> -rw------- 1 root root  538988 Oct 12 12:16 ./perf.data.2017101212165484
> -rw------- 1 root root  538988 Oct 12 12:16 ./perf.data.2017101212165586
> -rw------- 1 root root 1067812 Oct 12 12:16 ./perf.data.2017101212165653
> 
> You see the result keep getting larger because the ring buffer
> is never full in your case.

The increasing file size in my case indicates that the old processed data is dumped
into the new output.
I don't think it’s right. Because we should not process the same data multiple times.
That definitely increases the overhead of perf record. 


As digging deeper, I even suspect the overwrite mode doesn't really enabled
in perf record.
The overwrite is hard code to false.
	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages, false,
				 opts->auxtrace_mmap_pages,
				 opts->auxtrace_snapshot_mode) < 0) {
Did I miss something?


Thanks,
Kan