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: <20181005105557.GF3768@sejong>
Date:   Fri, 5 Oct 2018 19:55:57 +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

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