Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758416Ab2BIWlN (ORCPT ); Thu, 9 Feb 2012 17:41:13 -0500 Received: from mail-lpp01m010-f46.google.com ([209.85.215.46]:61139 "EHLO mail-lpp01m010-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753210Ab2BIWlM convert rfc822-to-8bit (ORCPT ); Thu, 9 Feb 2012 17:41:12 -0500 MIME-Version: 1.0 In-Reply-To: <4F344B0C.8060203@gmail.com> References: <1328826068-11713-1-git-send-email-eranian@google.com> <1328826068-11713-17-git-send-email-eranian@google.com> <4F344B0C.8060203@gmail.com> Date: Thu, 9 Feb 2012 23:41:10 +0100 Message-ID: Subject: Re: [PATCH v6 16/18] perf: enable reading of perf.data files from different ABI rev From: Stephane Eranian To: David Ahern Cc: linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@elte.hu, acme@redhat.com, robert.richter@amd.com, ming.m.lin@intel.com, andi@firstfloor.org, asharma@fb.com, ravitillo@lbl.gov, vweaver1@eecs.utk.edu, khandual@linux.vnet.ibm.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-System-Of-Record: true Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5716 Lines: 142 On Thu, Feb 9, 2012 at 11:39 PM, David Ahern wrote: > On 02/09/2012 03:21 PM, 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 >> --- >>  tools/perf/util/header.c  |   53 ++++++++++++++++++++++++++++++++++++++++---- >>  tools/perf/util/session.c |    4 +- >>  2 files changed, 50 insertions(+), 7 deletions(-) >> >> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c >> index 6f4187d..c2a3410 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) { >> +             pr_debug("cannot read %d bytes of header attr\n", >> +                      PERF_ATTR_SIZE_VER0); >> +             return -1; >> +     } >> + >> +     /* 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; >> +     } else if (sz > our_sz) { >> +             pr_debug("file uses a more recent and unsupported ABI" >> +                      " (%zu bytes extra)\n", sz - our_sz); >> +             return -1; >> +     } >> +     /* 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); >> +     } >> +     /* read perf_file_section, ids are read in caller */ >> +     ret = readn(fd, &f_attr->ids, sizeof(f_attr->ids)); > > I thought the above 2 lines were to be removed. > No, They are needed to get the offsets of the ids[] table. The field name are confusing. This is not reading the ids[], but just the offset of where the table is. Without that you get bogus offsets. I ran into that bug during testing... > David > > >> + >> +     return ret <= 0 ? -1 : 0; >> +} >> + >>  int perf_session__read_header(struct perf_session *session, int fd) >>  { >>       struct perf_header *header = &session->header; >> @@ -1974,19 +2019,17 @@ int perf_session__read_header(struct perf_session *session, int fd) >>       if (session->fd_pipe) >>               return perf_header__read_pipe(session, fd); >> >> -     if (perf_file_header__read(&f_header, header, fd) < 0) { >> -             pr_debug("incompatible file format\n"); >> +     if (perf_file_header__read(&f_header, header, fd) < 0) >>               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) >> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c >> index 3f3afed..5476320 100644 >> --- a/tools/perf/util/session.c >> +++ b/tools/perf/util/session.c >> @@ -24,7 +24,7 @@ static int perf_session__open(struct perf_session *self, bool force) >>               self->fd = STDIN_FILENO; >> >>               if (perf_session__read_header(self, self->fd) < 0) >> -                     pr_err("incompatible file format"); >> +                     pr_err("incompatible file format (rerun with -v to learn more)"); >> >>               return 0; >>       } >> @@ -56,7 +56,7 @@ static int perf_session__open(struct perf_session *self, bool force) >>       } >> >>       if (perf_session__read_header(self, self->fd) < 0) { >> -             pr_err("incompatible file format"); >> +             pr_err("incompatible file format (rerun with -v to learn more)"); >>               goto out_close; >>       } >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/