Re: [PATCH 14/42] perf record: Add --index option for building index table

[Adrian Hunter]

On 02/02/15 11:15, Jiri Olsa wrote:
> On Mon, Feb 02, 2015 at 10:34:50AM +0200, Adrian Hunter wrote:
> 
> SNIP
> 
>>> but how about bump up the header version for this feature? ;-)
>>>
>>> currently it's:
>>>
>>> struct perf_file_header {
>>>         u64                             magic;
>>>         u64                             size;
>>>         u64                             attr_size;
>>>         struct perf_file_section        attrs;
>>>         struct perf_file_section        data;
>>>         /* event_types is ignored */
>>>         struct perf_file_section        event_types;
>>>         DECLARE_BITMAP(adds_features, HEADER_FEAT_BITS);
>>> };
>>>
>>>
>>> - we already store attrs as a FEATURE so we could omit that
>>> - your patch stores only synthesized data into 'data' section (-1 idx)
>>>   this could be stored into separate file and get merged with the rest
>>> - new header version would have 'features' section, so the features
>>>   position wouldnt depend on the 'data' end as of now and we could
>>>   easily store after all data is merged:
>>>
>>> struct perf_file_header {
>>>         u64                             magic;
>>>         u64                             size;
>>>         u64                             attr_size;
>>>         struct perf_file_section        features;
>>>         DECLARE_BITMAP(adds_features, HEADER_FEAT_BITS);
>>> };
>>>
>>>
>>> thoughts?
>>
>> How come the features are being written before the sample data anyway?
>> I would have expected:
>> 	- write the data (update the index in memory)
>> 	- write the features (including index)
>>
> 
> I think the problem is that the only way how to get features offset
> right now is via perf_file_header::data.offset + perf_file_headerdata.size,
> and we still use this section to carry 'sythesized' data, so it needs
> to have correct size.

Why not make it the same as all the other data. i.e. find the start and size
via the index? And then just lump all the data together?

> I guess we could workaround that by storing the 'perf_file_header::data'
> as the last data section. That would require to treat it the same way as
> all other data sections, but we could keep current header layout.

Would it need to be last? Logically it should precede the data that depends
on it.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/