linux-kernel - Re: [PATCH v2 2/4] perf record: implement -z=<level> and --mmap-flush=<thres> options

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:   Wed, 20 Feb 2019 17:13:03 +0300
From:   Alexey Budankov <alexey.budankov@...ux.intel.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Andi Kleen <ak@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/4] perf record: implement -z=<level> and
 --mmap-flush=<thres> options


On 12.02.2019 16:08, Jiri Olsa wrote:
> On Mon, Feb 11, 2019 at 11:22:38PM +0300, Alexey Budankov wrote:
> 
> SNIP
> 
>>  static int process_synthesized_event(struct perf_tool *tool,
>>  				     union perf_event *event,
>>  				     struct perf_sample *sample __maybe_unused,

<SNIP>

>>  	for (i = 0; i < evlist->nr_mmaps; i++) {
>> +		u64 flush = MMAP_FLUSH_DEFAULT;
>>  		struct perf_mmap *map = &maps[i];
>>  
>>  		if (map->base) {
>>  			record__adjust_affinity(rec, map);
>> +			if (sync) {
>> +				flush = map->flush;
>> +				map->flush = MMAP_FLUSH_DEFAULT;
>> +			}
>>  			if (!record__aio_enabled(rec)) {
>>  				if (perf_mmap__push(map, rec, record__pushfn) != 0) {
>> +					if (sync)
>> +						map->flush = flush;
>>  					rc = -1;
>>  					goto out;
>>  				}
>> @@ -775,10 +814,14 @@ static int record__mmap_read_evlist(struct record *rec, struct perf_evlist *evli
>>  				idx = record__aio_sync(map, false);
>>  				if (perf_mmap__aio_push(map, rec, idx, record__aio_pushfn, &off) != 0) {
>>  					record__aio_set_pos(trace_fd, off);
>> +					if (sync)
>> +						map->flush = flush;
>>  					rc = -1;
>>  					goto out;
>>  				}
>>  			}
>> +			if (sync)
>> +				map->flush = flush;
>>  		}
> 
> what's 'flush' and 'sync' for? also the above handling seems confusing,
> why would you set it temporarily for default value if you let it set
> by user command line option?

flush is the threshold postponing and triggering the move of data to a 
trace file from the memory buffers. sync is the mean to force that move 
independently from the threshold value.

Despite a user provides flush value from the command line, the tool has 
to drain memory buffers, at least in the end of the collection, so that 
it is technically implemented like this.

> 
> SNIP
> 
>> -static void perf_mmap__aio_munmap(struct perf_mmap *map __maybe_unused)
>>  

<SNIP>

>> @@ -492,7 +518,7 @@ static int __perf_mmap__read_init(struct perf_mmap *md)
>>  	md->start = md->overwrite ? head : old;
>>  	md->end = md->overwrite ? old : head;
>>  
>> -	if (md->start == md->end)
>> +	if ((md->end - md->start) < md->flush)
>>  		return -EAGAIN;
> 
> we need document and explain this change in changelog in separate patch

Moved --mmap-flush option implementation into a separate patch.

Thanks,
Alexey

> 
> thanks,
> jirka
>