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

[Alexey Budankov]

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
>