Re: [PATCH v9 2/3]: perf record: enable asynchronous trace writing

[Alexey Budankov]

Hi,

On 05.10.2018 11:48, Namhyung Kim wrote:
> On Fri, Oct 05, 2018 at 11:31:11AM +0300, Alexey Budankov wrote:
<SNIP>
>>
>> Well, this could be implemented like this avoiding lseek() in else branch:
>>
>> 	off = lseek(trace_fd, 0, SEEK_CUR);
>> 	ret = record__aio_write(cblock, trace_fd, bf, size, off);
>> 	if (!ret) {
>> 		lseek(trace_fd, off + size, SEEK_SET);
>> 		rec->bytes_written += size;
>>
>> 		if (switch_output_size(rec))
>> 			trigger_hit(&switch_output_trigger);
>> 	}
> 
> Oh I meant the both like:
> 
> 	off = rec->bytes_written;
>  	ret = record__aio_write(cblock, trace_fd, bf, size, off);
>  	if (!ret) {
>  		rec->bytes_written += size;
> 
> 		...
> 

It still have to adjust the file pos thru lseek() prior leaving 
record__aio_pushfn() so space in trace file would be pre-allocated for 
enqueued record and file pos be moved beyond the record data, 
possibly for the next record.

> 
<SNIP>
> 
> Why not exposing opts.nr_cblocks regardless of the #ifdef?  It'll have
> 0 when it's not compiled in.  Then it could be like below (assuming
> you have all the dummy aio funcitons):
> 
> 
>>
>> 		if (map->base) {
>> 			if (!rec->opts.nr_cblocks) {
>> 				if (perf_mmap__push(map, rec, record__pushfn) != 0) {
>> 					rc = -1;
>> 					goto out;
>> 				}
>> 			} else {
>> 				int idx;
>> 				/*
>> 				 * Call record__aio_sync() to wait till map->data buffer
>> 				 * becomes available after previous aio write request.
>> 				 */
>> 				idx = record__aio_sync(map, false);
>> 				if (perf_mmap__aio_push(map, rec, idx, record__aio_pushfn) != 0) {
>> 					rc = -1;
>> 					goto out;
>> 				}
>> 			}
>> 		}
>>

Well, if it has AIO symbols + opts.nr_cblocks exposed unconditionally of 
HAVE_AIO_SUPPORT, but keeps the symbols implementation under the define, then
as far aio-cblocks option is not exposed thru command line, we end up in
whole bunch of symbols referenced under the else branch that, after all, 
can cause Perf binary size increase, which is, probably, worth avoiding.

Thanks,
Alexey