Hi Ingo,
On Tue, 15 Oct 2013 08:02:00 +0200, Ingo Molnar wrote:
> * David Ahern <dsah...@gmail.com> wrote:
>
>> + /* for MMAP based file writes */
>> + void *mmap_addr;
>> + u64 bytes_at_mmap_start; /* bytes in file when mmap
>> use starts */
>> + u64 mmap_offset; /* current location within mmap
>> */
>> + size_t mmap_size; /* size of mmap segments */
>> + bool use_mmap;
>
>> + if (!rec->opts.pipe_output && stat(output_name, &st) == 0) {
>> + rec->use_mmap = true;
>> + rec->bytes_at_mmap_start = st.st_size - rec->bytes_written;
>> + }
>
> 1)
>
> I think __cmd_record() has become way too large, nearly 300 lines of code.
> It would be nice to split it into 2-3 helpers that operate on 'struct
> perf_record' or so.
Agreed.
>
> 2)
>
> 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().
>
> 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).
>
> That means that this:
>
> rec->bytes_at_mmap_start = st.st_size - rec->bytes_written;
>
> is really:
>
> rec->bytes_at_mmap_start = st.st_size;
>
> furthermore, since we don't allow appends anymore, st.st_size ought to be
> zero as well.
>
> Which means that ->bytes_at_mmap_start is always zero - and could be
> eliminated altogether.
No, st_size is bigger than rec->bytes_written due to the
perf_file_header which is written without updating rec->bytes_written.
Actually I worried about the mmap offset not being aligned to page
size. But it seems that's not a problem.
Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
