From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S933155AbbBBJQz (ORCPT <rfc822;w@1wt.eu>);
	Mon, 2 Feb 2015 04:16:55 -0500
Received: from mx1.redhat.com ([209.132.183.28]:59230 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S933023AbbBBJQX (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Mon, 2 Feb 2015 04:16:23 -0500
Date: Mon, 2 Feb 2015 10:15:54 +0100
From: Jiri Olsa <jolsa@redhat.com>
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        LKML <linux-kernel@vger.kernel.org>, David Ahern <dsahern@gmail.com>,
        Andi Kleen <andi@firstfloor.org>,
        Stephane Eranian <eranian@google.com>,
        Frederic Weisbecker <fweisbec@gmail.com>
Subject: Re: [PATCH 14/42] perf record: Add --index option for building index
 table
Message-ID: <20150202091554.GA1404@krava.brq.redhat.com>
References: <1422518843-25818-1-git-send-email-namhyung@kernel.org>
 <1422518843-25818-15-git-send-email-namhyung@kernel.org>
 <20150201180635.GA6317@krava.brq.redhat.com>
 <54CF36AA.50508@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <54CF36AA.50508@intel.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.

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.

jirka