[PATCH] perf tools: Provide backward compatibility with previous perf.data version

[PATCH] perf tools: Provide backward compatibility with previous perf.data version

[Frederic Weisbecker]

We have merged the trace.info file into perf.data by adding one section
in the perf headers. This makes it incompatible with previous version:
the new perf tools can't read the older perf.data.

To support the previous format, we check the headers size. If they
have the same size than in the previous format, then ignore the trace
info section that doesn't exist.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
---
 tools/perf/util/header.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 212fade..9aae360 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -287,10 +287,16 @@ struct perf_header *perf_header__read(int fd)
 	do_read(fd, &f_header, sizeof(f_header));
 
 	if (f_header.magic	!= PERF_MAGIC		||
-	    f_header.size	!= sizeof(f_header)	||
 	    f_header.attr_size	!= sizeof(f_attr))
 		die("incompatible file format");
 
+	if (f_header.size != sizeof(f_header)) {
+		/* Support the previous format */
+		if (f_header.size == offsetof(typeof(f_header), trace_info))
+			f_header.trace_info.size = 0;
+		else
+			die("incompatible file format");
+	}
 	nr_attrs = f_header.attrs.size / sizeof(f_attr);
 	lseek(fd, f_header.attrs.offset, SEEK_SET);
 
-- 
1.6.2.3

Re: [PATCH] perf tools: Provide backward compatibility with previous perf.data version

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> We have merged the trace.info file into perf.data by adding one 
> section in the perf headers. This makes it incompatible with previous 
> version: the new perf tools can't read the older perf.data.
> 
> To support the previous format, we check the headers size. If they 
> have the same size than in the previous format, then ignore the trace 
> info section that doesn't exist.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Paul Mackerras <paulus@samba.org>
> ---
>  tools/perf/util/header.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 212fade..9aae360 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -287,10 +287,16 @@ struct perf_header *perf_header__read(int fd)
>  	do_read(fd, &f_header, sizeof(f_header));
>  
>  	if (f_header.magic	!= PERF_MAGIC		||
> -	    f_header.size	!= sizeof(f_header)	||
>  	    f_header.attr_size	!= sizeof(f_attr))
>  		die("incompatible file format");
>  
> +	if (f_header.size != sizeof(f_header)) {
> +		/* Support the previous format */
> +		if (f_header.size == offsetof(typeof(f_header), trace_info))
> +			f_header.trace_info.size = 0;
> +		else
> +			die("incompatible file format");
> +	}
>  	nr_attrs = f_header.attrs.size / sizeof(f_attr);
>  	lseek(fd, f_header.attrs.offset, SEEK_SET);

Perfect - thanks Frederic!

This should handle most of the (non-trace) perf.data's out there. 
trace.info is not something we really want to be backwards compatible 
about, if we can avoid it.

	Ingo

[tip:perf/core] perf tools: Provide backward compatibility with previous perf.data version

[tip-bot for Frederic Weisbecker]

Commit-ID:  26dd2cb074d9dc41c9e3cddd7bf175fd0a41febc
Gitweb:     http://git.kernel.org/tip/26dd2cb074d9dc41c9e3cddd7bf175fd0a41febc
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Thu, 8 Oct 2009 22:07:29 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 8 Oct 2009 22:11:02 +0200

perf tools: Provide backward compatibility with previous perf.data version

We have merged the trace.info file into perf.data by adding one
section in the perf headers. This makes it incompatible with
previous version: the new perf tools can't read the older
perf.data.

To support the previous format, we check the headers size. If they
have the same size than in the previous format, then ignore the
trace info section that doesn't exist.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
LKML-Reference: <1255032449-12022-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 tools/perf/util/header.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 212fade..9aae360 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -287,10 +287,16 @@ struct perf_header *perf_header__read(int fd)
 	do_read(fd, &f_header, sizeof(f_header));
 
 	if (f_header.magic	!= PERF_MAGIC		||
-	    f_header.size	!= sizeof(f_header)	||
 	    f_header.attr_size	!= sizeof(f_attr))
 		die("incompatible file format");
 
+	if (f_header.size != sizeof(f_header)) {
+		/* Support the previous format */
+		if (f_header.size == offsetof(typeof(f_header), trace_info))
+			f_header.trace_info.size = 0;
+		else
+			die("incompatible file format");
+	}
 	nr_attrs = f_header.attrs.size / sizeof(f_attr);
 	lseek(fd, f_header.attrs.offset, SEEK_SET);
 

Re: [PATCH] perf tools: Provide backward compatibility with previous perf.data version

[Frederic Weisbecker]

On Thu, Oct 08, 2009 at 10:07:29PM +0200, Frederic Weisbecker wrote:
> We have merged the trace.info file into perf.data by adding one section
> in the perf headers. This makes it incompatible with previous version:
> the new perf tools can't read the older perf.data.
> 
> To support the previous format, we check the headers size. If they
> have the same size than in the previous format, then ignore the trace
> info section that doesn't exist.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Paul Mackerras <paulus@samba.org>
> ---
>  tools/perf/util/header.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 212fade..9aae360 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -287,10 +287,16 @@ struct perf_header *perf_header__read(int fd)
>  	do_read(fd, &f_header, sizeof(f_header));
>  
>  	if (f_header.magic	!= PERF_MAGIC		||
> -	    f_header.size	!= sizeof(f_header)	||



Few notes about this.
I can send a separate fix for .32 that would just consist in the above line,
or more likely I can replace it by:

	f_header.size < sizeof(f_header)     ||

and then we'll get a minimal forward compatibility from the older
tools (can be Cc'ed to stable for .31).

Another thing. We may feel the need to add yet another sections
in the future.

So just a suggestion: we could turn this trace_info section into a
more generic one in which we could add as much subsections as we want
in the future while always ensuring backward compatibility. That could
be managed through a versioning of this generic section.

Of course I don't wish too much format modifications
will happen in the future, because it means difficults interaction
between several perf tools versions.

That said, it could still happen if important new needs arise and
I guess we don't want to be blocked by the header size.

The other solution is to continue like I did in this patch: guess the
version of this file format against its size.

Thanks.

Re: [PATCH] perf tools: Provide backward compatibility with previous perf.data version

[Frederic Weisbecker]

On Thu, Oct 08, 2009 at 10:12:22PM +0200, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > We have merged the trace.info file into perf.data by adding one 
> > section in the perf headers. This makes it incompatible with previous 
> > version: the new perf tools can't read the older perf.data.
> > 
> > To support the previous format, we check the headers size. If they 
> > have the same size than in the previous format, then ignore the trace 
> > info section that doesn't exist.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Mike Galbraith <efault@gmx.de>
> > Cc: Paul Mackerras <paulus@samba.org>
> > ---
> >  tools/perf/util/header.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> > 
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 212fade..9aae360 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -287,10 +287,16 @@ struct perf_header *perf_header__read(int fd)
> >  	do_read(fd, &f_header, sizeof(f_header));
> >  
> >  	if (f_header.magic	!= PERF_MAGIC		||
> > -	    f_header.size	!= sizeof(f_header)	||
> >  	    f_header.attr_size	!= sizeof(f_attr))
> >  		die("incompatible file format");
> >  
> > +	if (f_header.size != sizeof(f_header)) {
> > +		/* Support the previous format */
> > +		if (f_header.size == offsetof(typeof(f_header), trace_info))
> > +			f_header.trace_info.size = 0;
> > +		else
> > +			die("incompatible file format");
> > +	}
> >  	nr_attrs = f_header.attrs.size / sizeof(f_attr);
> >  	lseek(fd, f_header.attrs.offset, SEEK_SET);
> 
> Perfect - thanks Frederic!
> 
> This should handle most of the (non-trace) perf.data's out there. 


Yeah, hopefully. I've only tested in a simple old perf.data but
that should do the trick for all of the non-trace files.


> trace.info is not something we really want to be backwards compatible 
> about, if we can avoid it.
> 
> 	Ingo


Yeah, that said, it's still possible and wouldn't require too much changes.
May be let's wait for complaints and, if any, I can provide this backward
support too.

Thanks.

Re: [PATCH] perf tools: Provide backward compatibility with previous perf.data version

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Thu, Oct 08, 2009 at 10:12:22PM +0200, Ingo Molnar wrote:
> > 
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > We have merged the trace.info file into perf.data by adding one 
> > > section in the perf headers. This makes it incompatible with previous 
> > > version: the new perf tools can't read the older perf.data.
> > > 
> > > To support the previous format, we check the headers size. If they 
> > > have the same size than in the previous format, then ignore the trace 
> > > info section that doesn't exist.
> > > 
> > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > Cc: Mike Galbraith <efault@gmx.de>
> > > Cc: Paul Mackerras <paulus@samba.org>
> > > ---
> > >  tools/perf/util/header.c |    8 +++++++-
> > >  1 files changed, 7 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > > index 212fade..9aae360 100644
> > > --- a/tools/perf/util/header.c
> > > +++ b/tools/perf/util/header.c
> > > @@ -287,10 +287,16 @@ struct perf_header *perf_header__read(int fd)
> > >  	do_read(fd, &f_header, sizeof(f_header));
> > >  
> > >  	if (f_header.magic	!= PERF_MAGIC		||
> > > -	    f_header.size	!= sizeof(f_header)	||
> > >  	    f_header.attr_size	!= sizeof(f_attr))
> > >  		die("incompatible file format");
> > >  
> > > +	if (f_header.size != sizeof(f_header)) {
> > > +		/* Support the previous format */
> > > +		if (f_header.size == offsetof(typeof(f_header), trace_info))
> > > +			f_header.trace_info.size = 0;
> > > +		else
> > > +			die("incompatible file format");
> > > +	}
> > >  	nr_attrs = f_header.attrs.size / sizeof(f_attr);
> > >  	lseek(fd, f_header.attrs.offset, SEEK_SET);
> > 
> > Perfect - thanks Frederic!
> > 
> > This should handle most of the (non-trace) perf.data's out there. 
> 
> 
> Yeah, hopefully. I've only tested in a simple old perf.data but that 
> should do the trick for all of the non-trace files.

yeah, i tested it on an old perf.data too, and it worked.

> > trace.info is not something we really want to be backwards compatible 
> > about, if we can avoid it.
> > 
> > 	Ingo
> 
> 
> Yeah, that said, it's still possible and wouldn't require too much 
> changes. May be let's wait for complaints and, if any, I can provide 
> this backward support too.

ok.

	Ingo

Re: [PATCH] perf tools: Provide backward compatibility with previous perf.data version

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Thu, Oct 08, 2009 at 10:07:29PM +0200, Frederic Weisbecker wrote:
> > We have merged the trace.info file into perf.data by adding one section
> > in the perf headers. This makes it incompatible with previous version:
> > the new perf tools can't read the older perf.data.
> > 
> > To support the previous format, we check the headers size. If they
> > have the same size than in the previous format, then ignore the trace
> > info section that doesn't exist.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Mike Galbraith <efault@gmx.de>
> > Cc: Paul Mackerras <paulus@samba.org>
> > ---
> >  tools/perf/util/header.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> > 
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 212fade..9aae360 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -287,10 +287,16 @@ struct perf_header *perf_header__read(int fd)
> >  	do_read(fd, &f_header, sizeof(f_header));
> >  
> >  	if (f_header.magic	!= PERF_MAGIC		||
> > -	    f_header.size	!= sizeof(f_header)	||
> 
> 
> 
> Few notes about this.
> I can send a separate fix for .32 that would just consist in the above line,
> or more likely I can replace it by:
> 
> 	f_header.size < sizeof(f_header)     ||
> 
> and then we'll get a minimal forward compatibility from the older
> tools (can be Cc'ed to stable for .31).

Yep, would be nice.

> Another thing. We may feel the need to add yet another sections in the 
> future.
> 
> So just a suggestion: we could turn this trace_info section into a 
> more generic one in which we could add as much subsections as we want 
> in the future while always ensuring backward compatibility. That could 
> be managed through a versioning of this generic section.

Definitely! (Since it's not upstream yet we can do it without supporting 
the interim version.)

	Ingo