Messages in this thread | | | Date | Tue, 15 Oct 2013 07:25:04 -0600 | From | David Ahern <> | Subject | Re: [PATCH] perf record: mmap output file - v2 |
| |
On 10/15/13 1:09 AM, Namhyung Kim wrote: >> The stat() seems superfluous, here in __cmd_record() we've just checked >> the output_name and made sure it exists. Can that stat() call ever fail? > > AFAICS it's needed to check current file size. But I think it's better > to use fstat().
Sure fstat could be used over stat -- if it ends up staying.
>> >> 3) >> >> The rec->bytes_at_mmap_start field feels a bit weird. If I read the code >> correctly, in every 'perf record' invocation, rec->bytes_written starts at >> 0 - i.e. we don't have repeat invocations of cmd_record(). > > rec->bytes_written is updated when it writes to the output file for > synthesizing COMM/MMAP events (this mmap output is not used at that time).
Ingo: I went through a number of itereations before using the bytes_at_mmap_start. One of those was to use the bytes_written counter. All failed. Header + synthesized events are written to the file before we start farming the ring buffers.
Perhaps a good code cleanup will help figure out why. I needed the functionality ASAP for use with perf-trace -a so I stuck with the new variable. Since this change is working out well, I will look at a code clean up on the next round.
I am traveling to LinuxCon / KVM Forum / Tracing Forum on Friday. Perhaps the clean up and followup patch can be done on the long plane ride; more likely when I return which means 3.14 material.
> Actually I worried about the mmap offset not being aligned to page > size. But it seems that's not a problem.
This code snippet makes sure the mmap offset is a multiple of 64M (rec->mmap_size). offset is the argument to mmap; mmap_offset is the where we are within the mmap for the next copy:
+ offset = rec->bytes_at_mmap_start + rec->bytes_written; + if (offset < (ssize_t) rec->mmap_size) { + rec->mmap_offset = offset; + offset = 0; + } else + rec->mmap_offset = 0;
David
| |