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.
>
>
>> + }
>> + } 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);
}
>
>
>> + 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?
if (map->base) {
#ifdef HAVE_AIO_SUPPORT
if (!rec->opts.nr_cblocks) {
#endif
if (perf_mmap__push(map, rec, record__pushfn)
!= 0) {
rc = -1;
goto out;
}
#ifdef HAVE_AIO_SUPPORT
} 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;
}
}
#endif
}
Thanks,
Alexey
