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

[Namhyung Kim <namhyung@kernel.org>]

Hi,

On Fri, Oct 05, 2018 at 12:39:10PM +0300, Alexey Budankov wrote:
> 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.

For that purpose, isn't it better calling ftruncate() with a
reasonable batch size to reduce number of syscalls?


> 
> > 
> <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.

I think it's ok as long as they're empty.

Thanks,
Namhyung