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: <4ee1c347-674b-ac61-65cf-55fb71a7cc2b@linux.intel.com>
Date:   Fri, 5 Oct 2018 12:39:10 +0300
From:   Alexey Budankov <alexey.budankov@...ux.intel.com>
To:     Namhyung Kim <namhyung@...nel.org>
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 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