Re: [PATCH v7 1/2]: perf util: map data buffer for preserving collected data

[Alexey Budankov <alexey.budankov@linux.intel.com>]

On 06.09.2018 14:04, Jiri Olsa wrote:
> On Wed, Sep 05, 2018 at 10:19:56AM +0300, Alexey Budankov wrote:
>>
>> The map->data buffers are used to preserve map->base profiling data 
>> for writing to disk. AIO map->cblocks are used to queue corresponding 
>> map->data buffers for asynchronous writing. map->cblocks objects are 
>> located in the last page of every map->data buffer.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>>  Changes in v7:
>>   - implemented handling record.aio setting from perfconfig file
>>  Changes in v6:
>>   - adjusted setting of priorities for cblocks;
>>  Changes in v5:
>>   - reshaped layout of data structures;
>>   - implemented --aio option;
>>  Changes in v4:
>>   - converted mmap()/munmap() to malloc()/free() for mmap->data buffer management 
>>  Changes in v2:
>>   - converted zalloc() to calloc() for allocation of mmap_aio array,
>>   - cleared typo and adjusted fallback branch code;
>> ---
>>  tools/perf/builtin-record.c | 15 ++++++++++++-
>>  tools/perf/perf.h           |  1 +
>>  tools/perf/util/evlist.c    |  7 +++---
>>  tools/perf/util/evlist.h    |  3 ++-
>>  tools/perf/util/mmap.c      | 53 +++++++++++++++++++++++++++++++++++++++++++++
>>  tools/perf/util/mmap.h      |  6 ++++-
>>  6 files changed, 79 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index 22ebeb92ac51..f17a6f9cb1ba 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -326,7 +326,8 @@ static int record__mmap_evlist(struct record *rec,
>>  
>>  	if (perf_evlist__mmap_ex(evlist, opts->mmap_pages,
>>  				 opts->auxtrace_mmap_pages,
>> -				 opts->auxtrace_snapshot_mode) < 0) {
>> +				 opts->auxtrace_snapshot_mode,
>> +				 opts->nr_cblocks) < 0) {
>>  		if (errno == EPERM) {
>>  			pr_err("Permission error mapping pages.\n"
>>  			       "Consider increasing "
>> @@ -1287,6 +1288,8 @@ static int perf_record_config(const char *var, const char *value, void *cb)
>>  		var = "call-graph.record-mode";
>>  		return perf_default_config(var, value, cb);
>>  	}
>> +	if (!strcmp(var, "record.aio"))
>> +		rec->opts.nr_cblocks = strtol(value, NULL, 0);
>>  
>>  	return 0;
>>  }
>> @@ -1519,6 +1522,7 @@ static struct record record = {
>>  			.default_per_cpu = true,
>>  		},
>>  		.proc_map_timeout     = 500,
>> +		.nr_cblocks	      = 2
>>  	},
>>  	.tool = {
>>  		.sample		= process_sample_event,
>> @@ -1678,6 +1682,8 @@ static struct option __record_options[] = {
>>  			  "signal"),
>>  	OPT_BOOLEAN(0, "dry-run", &dry_run,
>>  		    "Parse options then exit"),
>> +	OPT_INTEGER(0, "aio", &record.opts.nr_cblocks,
>> +		    "asynchronous trace write operations (min: 1, max: 32, default: 2)"),
> 
> ok, so this got silently added in recent versions and I couldn't
> find any justification for it.. why do we use more aio blocks for
> single map now? also why the default is 2?

Having more blocks may improve thruput from kernel to userspace for 
cases when we get more data at map->base but the started AIO is not 
finished yet. That can easily happen between calls of 
record__mmap_read_evlist().

> 
> the option should be more specific like 'aio-blocks'

ok.

> 
> the change is difficult enough.. we should start simple and add
> these additions with proper justification in separate patches

Setting default to 1 gives the simplest solution. I could provide 
justification where spinning at record__aio_sync() becomes the hotspot.

> 
> thanks,
> jirka
>