linux-kernel - Re: [PATCH v2 3/4] perf tools: Support reading from backward ring buffer

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]

Date:	Fri, 6 May 2016 21:40:52 +0800
From:	"Wangnan (F)" <wangnan0@...wei.com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
CC:	<linux-kernel@...r.kernel.org>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Zefan Li <lizefan@...wei.com>, <pi3orama@....com>
Subject: Re: [PATCH v2 3/4] perf tools: Support reading from backward ring
 buffer



On 2016/5/6 4:07, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 27, 2016 at 02:19:22AM +0000, Wang Nan escreveu:
>> perf_evlist__mmap_read_backward() is introduced for reading backward
>> ring buffer. Different from reading forward, before reading, caller
>> needs to call perf_evlist__mmap_read_catchup() first.
>>
>> Backward ring buffer should be read from 'head' pointer, not '0'.
>> perf_evlist__mmap_read_catchup() saves 'head' to 'md->prev', then
>> make it remember the start position after each reading.
>>
>> Signed-off-by: Wang Nan <wangnan0@...wei.com>
>> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Zefan Li <lizefan@...wei.com>
>> Cc: pi3orama@....com
>> ---
>>   tools/perf/util/evlist.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   tools/perf/util/evlist.h |  4 ++++
>>   2 files changed, 43 insertions(+)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 17cd014..2e0b7b0 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -766,6 +766,45 @@ union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx)
>>   	return perf_mmap__read(md, evlist->overwrite, old, head, &md->prev);
>>   }
>>   
>> +union perf_event *
>> +perf_evlist__mmap_read_backward(struct perf_evlist *evlist, int idx)
>> +{
>> +	struct perf_mmap *md = &evlist->mmap[idx];
>> +	u64 head, end;
>> +	u64 start = md->prev;
>> +
>> +	/*
>> +	 * Check if event was unmapped due to a POLLHUP/POLLERR.
>> +	 */
>> +	if (!atomic_read(&md->refcnt))
>> +		return NULL;
>> +
>> +	/* NOTE: head is negative in this case */
> What is this comment for, can you ellaborate? Are you double sure this
> arithmetic with u64, negative values, that -head below are all ok?

Yes. In backward ring buffer, kernel write data from '0'. Each
time it write a record, it minus sizeof(record) from 'head'
pointer. So '-head' means number of bytes already written.

> I've applied the first two patches in this series.
>
> I also need to check why we now need that catchup thing :-\

I think catchup is necessary. I tried to get rid of it by setting
md->prev to -1 and initialize it to 0 when reading forwardly and
to head when reading backwardly, but it require too much code
adjustments.

>> +	head = perf_mmap__read_head(md);
>> +
>> +	if (!head)
>> +		return NULL;
>> +
>> +	end = head + md->mask + 1;
>> +
>> +	if ((end - head) > -head)
>> +		end = 0;
>> +

This '-head' is used to detect wrapping. Never use positive pointer.

>> +	return perf_mmap__read(md, false, start, end, &md->prev);
>> +}
>> +
>> +void perf_evlist__mmap_read_catchup(struct perf_evlist *evlist, int idx)
>> +{
>> +	struct perf_mmap *md = &evlist->mmap[idx];
>> +	u64 head;
>> +
>> +	if (!atomic_read(&md->refcnt))
>> +		return;
>> +
>> +	head = perf_mmap__read_head(md);
>> +	md->prev = head;
>> +}
>> +
>>   static bool perf_mmap__empty(struct perf_mmap *md)
>>   {
>>   	return perf_mmap__read_head(md) == md->prev && !md->auxtrace_mmap.base;
>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
>> index 208897a..85d1b59 100644
>> --- a/tools/perf/util/evlist.h
>> +++ b/tools/perf/util/evlist.h
>> @@ -129,6 +129,10 @@ struct perf_sample_id *perf_evlist__id2sid(struct perf_evlist *evlist, u64 id);
>>   
>>   union perf_event *perf_evlist__mmap_read(struct perf_evlist *evlist, int idx);
>>   
>> +union perf_event *perf_evlist__mmap_read_backward(struct perf_evlist *evlist,
>> +						  int idx);
>> +void perf_evlist__mmap_read_catchup(struct perf_evlist *evlist, int idx);
>> +
>>   void perf_evlist__mmap_consume(struct perf_evlist *evlist, int idx);
>>   
>>   int perf_evlist__open(struct perf_evlist *evlist);
>> -- 
>> 1.8.3.4