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

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]

Message-ID: <20181005084854.GE3768@sejong>
Date:   Fri, 5 Oct 2018 17:48:54 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Alexey Budankov <alexey.budankov@...ux.intel.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>, Andi Kleen <ak@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, kernel-team@....com
Subject: Re: [PATCH v9 2/3]: perf record: enable asynchronous trace writing

On Fri, Oct 05, 2018 at 11:31:11AM +0300, Alexey Budankov wrote:
> Hi,

Hi,

> 
> On 05.10.2018 10:16, Namhyung Kim wrote:
> > On Wed, Oct 03, 2018 at 07:12:10PM +0300, Alexey Budankov wrote:
> <SNIP>
> >> +static void record__aio_sync(struct perf_mmap *md)
> >> +{
> >> +	struct aiocb *cblock = &md->cblock;
> >> +	struct timespec timeout = { 0, 1000 * 1000  * 1 }; // 1ms
> >> +
> >> +	do {
> >> +		if (cblock->aio_fildes == -1 || record__aio_complete(md, cblock))
> >> +			return;
> >> +
> >> +		while (aio_suspend((const struct aiocb**)&cblock, 1, &timeout)) {
> >> +			if (!(errno == EAGAIN || errno == EINTR))
> >> +				pr_err("failed to sync perf data, error: %m\n");
> > 
> > Is there somthing we can do in this error case?  Any chance it gets
> > stuck in the loop?
> 
> Not really. Currently, in glibc, it can block on a mutex only.

OK, I was thinking of a situation where the aio_suspend() keeps
returning a same error code repeated..


> 
> > 
> > 
> >> +		}
> >> +	} while (1);
> >> +}
> >> +
> >> +static int record__aio_pushfn(void *to, struct aiocb *cblock, void *bf, size_t size)
> >> +{
> >> +	off_t off;
> >> +	struct record *rec = to;
> >> +	int ret, trace_fd = rec->session->data->file.fd;
> >> +
> >> +	rec->samples++;
> >> +
> >> +	off = lseek(trace_fd, 0, SEEK_CUR);
> >> +	lseek(trace_fd, off + size, SEEK_SET);
> > 
> > It'd be nice if these lseek() could be removed and use
> > rec->bytes_written instead.
> 
> 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;

		...


> > 
> > 
> >> +	ret = record__aio_write(cblock, trace_fd, bf, size, off);
> >> +	if (!ret) {
> >> +		rec->bytes_written += size;
> >> +
> >> +		if (switch_output_size(rec))
> >> +			trigger_hit(&switch_output_trigger);
> > 
> > Doesn't it need the _sync() before the trigger?  Maybe it should be
> > moved to record__mmap_read_evlist() or so..
> 
> Currently trigger just updates variable state.
> The state is then checked thru separate API at __cmd_record() where 
> record__mmap_read_sync() is called prior switching to a new trace file 
> or finishing collection.
> 
> >>  
> <SNIP>
> >>  		if (map->base) {
> >> +#ifndef HAVE_AIO_SUPPORT
> >>  			if (perf_mmap__push(map, rec, record__pushfn) != 0) {
> >>  				rc = -1;
> >>  				goto out;
> >>  			}
> >> +#else
> >> +			if (!rec->opts.nr_cblocks) {
> >> +				if (perf_mmap__push(map, rec, record__pushfn) != 0) {
> >> +					rc = -1;
> >> +					goto out;
> >> +				}
> >> +			} else {
> >> +				/*
> >> +				 * Call record__aio_sync() to wait till map->data buffer
> >> +				 * becomes available after previous aio write request.
> >> +				 */
> >> +				record__aio_sync(map);
> >> +				if (perf_mmap__aio_push(map, rec, record__aio_pushfn) != 0) {
> >> +					rc = -1;
> >> +					goto out;
> >> +				}
> >> +			}
> >> +#endif
> > 
> > If dummy aio functions are provided, the #ifdef can be removed and
> > just use the #else part assuming opts.nr_cblocks == 0.
> 
> Yes, it looks a little bit cumbersome. Would this be more compact?

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;
> 				}
> 			}
> 		}
> 

Thanks,
Namhyung