Re: [PATCH v5 16/18] perf: enable reading of perf.data files fromdifferent ABI rev

From: Stephane Eranian
Date: Tue Feb 07 2012 - 10:50:49 EST


On Mon, Feb 6, 2012 at 11:19 PM, David Ahern <dsahern@xxxxxxxxx> wrote:
>
>
> On 02/02/2012 05:54 AM, Stephane Eranian wrote:
>> This patch allows perf to process perf.data files generated
>> using an ABI that has a different perf_event_attr struct size, i.e.,
>> a different ABI version.
>>
>> The perf_event_attr can be extended, yet perf needs to cope with
>> older perf.data files. Similarly, perf must be able to cope with
>> a perf.data file which is using a newer version of the ABI than
>> what it knows about.
>>
>> This patch adds read_attr(), a routine that reads a perf_event_attr
>> struct from a file incrementally based on its advertised size. If
>> the on-file struct is smaller than what perf knows, then the extra
>> fields are zeroed. If the on-file struct is bigger, then perf only
>> uses what it knows about, the rest is skipped.
>>
>> Signed-off-by: Stephane Eranian <eranian@xxxxxxxxxx>
>> ---
>> Âtools/perf/util/header.c | Â 49 ++++++++++++++++++++++++++++++++++++++++++++-
>> Â1 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index 6f4187d..8d6c18d 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -1959,6 +1959,51 @@ static int perf_header__read_pipe(struct perf_session *session, int fd)
>> Â Â Â return 0;
>> Â}
>>
>> +static int read_attr(int fd, struct perf_header *ph,
>> + Â Â Â Â Â Â Â Â Âstruct perf_file_attr *f_attr)
>> +{
>> + Â Â struct perf_event_attr *attr = &f_attr->attr;
>> + Â Â size_t sz, left;
>> + Â Â size_t our_sz = sizeof(f_attr->attr);
>> + Â Â int ret;
>> +
>> + Â Â memset(f_attr, 0, sizeof(*f_attr));
>> +
>> + Â Â /* read minimal guaranteed structure */
>> + Â Â ret = readn(fd, attr, PERF_ATTR_SIZE_VER0);
>> + Â Â if (ret <= 0)
>> + Â Â Â Â Â Â return -1;
>
> As I recall the first bump in that structure happened in 2.6.32. Why add
> backward compatibility for it now? ie., why not just expect VER1
>
>> +
>> + Â Â /* on file perf_event_attr size */
>> + Â Â sz = attr->size;
>> + Â Â if (ph->needs_swap)
>> + Â Â Â Â Â Â sz = bswap_32(sz);
>> +
>> + Â Â if (sz == 0) {
>> + Â Â Â Â Â Â /* assume ABI0 */
>> + Â Â Â Â Â Â sz = ÂPERF_ATTR_SIZE_VER0;
>
> Shouldn't this be a failure? ie., problem with the file (or the
> swapping) since size can't be 0
>
size can be zero. In which case, it means ABI0 version.
See kernel/event/core.c:perf_copy_attr().


> And then for the following why not restrict sz to known, expected sizes
> -- using the PERF_ATTR_SIZE_VER defines introduced in patch 15?
>
Well, the current code solves the problem once and for all. Old tools
can still read new files and vice-versa. If you think that's a problem I
can simply bail out if sz > our_sz.

>> + Â Â } else if (sz > our_sz) {
>> + Â Â Â Â Â Â /* bigger than what we know about */
>> + Â Â Â Â Â Â sz = our_sz;
>> +
>> + Â Â Â Â Â Â /* skip what we do not know about */
>> + Â Â Â Â Â Â lseek(fd, SEEK_CUR, attr->size - our_sz);
>> + Â Â }
>> + Â Â /* what we have not yet read and that we know about */
>> + Â Â left = sz - PERF_ATTR_SIZE_VER0;
>> + Â Â if (left) {
>> + Â Â Â Â Â Â void *ptr = attr;
>> + Â Â Â Â Â Â ptr += PERF_ATTR_SIZE_VER0;
>> +
>> + Â Â Â Â Â Â ret = readn(fd, ptr, left);
>> + Â Â Â Â Â Â if (ret <= 0)
>> + Â Â Â Â Â Â Â Â Â Â return -1;
>> + Â Â }
>> + Â Â /* read the ids */
>> + Â Â ret = readn(fd, &f_attr->ids, sizeof(struct perf_file_section));
>
> Confused by the above? It is not done in the old code, so why read the
> ids here? I scanned the other patches, but don't see other code movement
> on this file.
>
Good catch. It is leftover code from debugging most likely. The ids are read
later on in perf_session__read_header().

> David
>
>> + Â Â return ret <= 0 ? -1 : 0;
>> +}
>> +
>> Âint perf_session__read_header(struct perf_session *session, int fd)
>> Â{
>> Â Â Â struct perf_header *header = &session->header;
>> @@ -1979,14 +2024,14 @@ int perf_session__read_header(struct perf_session *session, int fd)
>> Â Â Â Â Â Â Â return -EINVAL;
>> Â Â Â }
>>
>> - Â Â nr_attrs = f_header.attrs.size / sizeof(f_attr);
>> + Â Â nr_attrs = f_header.attrs.size / f_header.attr_size;
>> Â Â Â lseek(fd, f_header.attrs.offset, SEEK_SET);
>>
>> Â Â Â for (i = 0; i < nr_attrs; i++) {
>> Â Â Â Â Â Â Â struct perf_evsel *evsel;
>> Â Â Â Â Â Â Â off_t tmp;
>>
>> - Â Â Â Â Â Â if (readn(fd, &f_attr, sizeof(f_attr)) <= 0)
>> + Â Â Â Â Â Â if (read_attr(fd, header, &f_attr) < 0)
>> Â Â Â Â Â Â Â Â Â Â Â goto out_errno;
>>
>> Â Â Â Â Â Â Â if (header->needs_swap)
--
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/