| 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
| ||
|
Message-ID: <f82cd135-7012-7009-247b-3f76fa70ea94@huawei.com>
Date: Thu, 12 Oct 2017 09:11:57 +0800
From: "Wangnan (F)" <wangnan0@...wei.com>
To: "Liang, Kan" <kan.liang@...el.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
On 2017/10/11 22:57, Liang, Kan wrote:
>>>>>> 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.
>>
> For the issue, I mentioned above.
> What do think about the patch as below?
> It tries to avoid the duplicate data.
>
> From 8b058ea6977a97e5705aa2f64bdd014fd76d1247 Mon Sep 17 00:00:00 2001
> From: Kan Liang <Kan.liang@...el.com>
> Date: Wed, 11 Oct 2017 07:39:34 -0700
> Subject: [PATCH] perf tool: fix: Don't discard prev in backward mode
>
> Perf record can switch output. The new output should only store the data
> after switching. However, in overwrite backward mode, the new output
> still have the data from old output. That also brings extra overhead.
>
> At the end of mmap_read, the position of processed ring buffer is saved
> in md->prev. Next mmap_read should be end in md->prev if it is not
> overwriten. That avoids to process duplicate data.
> However, the md->prev is discarded. So next mmap_read has to process
> whole valid ring buffer, which probably include the old processed
> data.
>
> Introduce fast path for backward_rb_find_range. Stop searching when
> md->prev is detected.
>
> Signed-off-by: Kan Liang <Kan.liang@...el.com>
> ---
> tools/perf/util/mmap.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/mmap.c b/tools/perf/util/mmap.c
> index 9fe5f9c..36b459a 100644
> --- a/tools/perf/util/mmap.c
> +++ b/tools/perf/util/mmap.c
> @@ -254,7 +254,8 @@ int perf_mmap__mmap(struct perf_mmap *map, struct mmap_params *mp, int fd)
> return 0;
> }
>
> -static int backward_rb_find_range(void *buf, int mask, u64 head, u64 *start, u64 *end)
> +static int backward_rb_find_range(void *buf, int mask, u64 head,
> + u64 old, u64 *start, u64 *end)
> {
> struct perf_event_header *pheader;
> u64 evt_head = head;
> @@ -282,6 +283,12 @@ static int backward_rb_find_range(void *buf, int mask, u64 head, u64 *start, u64
>
> evt_head += pheader->size;
> pr_debug3("move evt_head: %"PRIx64"\n", evt_head);
> +
> + /* fast path: avoid to process duplicate data */
> + if (old == evt_head) {
> + *end = evt_head;
> + return 0;
> + }
You still need to parse the whole ring buffer.
You can use 'old - head' to check how many bytes are written into
the ring buffer since you last read. If its size is less than the
ring buffer size, there's no overwriting happen. In this case a
simple copy should be enough.
Thank you.
Powered by blists - more mailing lists