Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 13 Jan 2021 10:27:32 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Mike Leach <mike.leach@linaro.org>
Cc:     Suzuki K Poulose <suzuki.poulose@arm.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        John Garry <john.garry@huawei.com>,
        Will Deacon <will@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Daniel Kiss <Daniel.Kiss@arm.com>,
        Denis Nikitin <denik@chromium.org>,
        Coresight ML <coresight@lists.linaro.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 3/7] perf cs-etm: Calculate per CPU metadata array size
Message-ID: <20210113022721.GG18965@leoy-ThinkPad-X240s>
References: <20210109074435.626855-1-leo.yan@linaro.org>
 <20210109074435.626855-4-leo.yan@linaro.org>
 <96ec434e-4103-02ac-a05a-761a9ca8cb0d@arm.com>
 <CAJ9a7VjtUuRRYBBu63kSXKwrGdB8ZoWJz-bE1g9tMLSbkFVDGg@mail.gmail.com>
 <20210111150608.GC222747@leoy-ThinkPad-X240s>
 <CAJ9a7VjhMOq=r=W2xtjgiHhmvwk1xDzMnWF4hKPogX8PX+ZADg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJ9a7VjhMOq=r=W2xtjgiHhmvwk1xDzMnWF4hKPogX8PX+ZADg@mail.gmail.com>
Precedence: bulk

Hi Mike,

On Wed, Jan 13, 2021 at 12:00:10AM +0000, Mike Leach wrote:

[...]

> > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> > index a2a369e2fbb6..edaec57362f0 100644
> > --- a/tools/perf/util/cs-etm.c
> > +++ b/tools/perf/util/cs-etm.c
> > @@ -2558,12 +2558,19 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
> >                                 err = -ENOMEM;
> >                                 goto err_free_metadata;
> >                         }
> > -                       for (k = 0; k < CS_ETM_PRIV_MAX; k++)
> > +                       for (k = 0; k < CS_ETM_PRIV_MAX; k++) {
> >                                 metadata[j][k] = ptr[i + k];
> >
> > +                               if (ptr[i + k + 1] == __perf_cs_etmv3_magic ||
> > +                                   ptr[i + k + 1] == __perf_cs_etmv4_magic) {
> > +                                       k++;
> > +                                       break;
> > +                               }
> > +                       }
> > +
> >                         /* The traceID is our handle */
> >                         idx = metadata[j][CS_ETM_ETMTRACEIDR];
> > -                       i += CS_ETM_PRIV_MAX;
> > +                       i += k;
> >                 } else if (ptr[i] == __perf_cs_etmv4_magic) {
> >                         metadata[j] = zalloc(sizeof(*metadata[j]) *
> >                                              CS_ETMV4_PRIV_MAX);
> > @@ -2571,12 +2578,19 @@ int cs_etm__process_auxtrace_info(union perf_event *event,
> >                                 err = -ENOMEM;
> >                                 goto err_free_metadata;
> >                         }
> > -                       for (k = 0; k < CS_ETMV4_PRIV_MAX; k++)
> > +                       for (k = 0; k < CS_ETMV4_PRIV_MAX; k++) {
> >                                 metadata[j][k] = ptr[i + k];
> >
> > +                               if (ptr[i + k + 1] == __perf_cs_etmv3_magic ||
> > +                                   ptr[i + k + 1] == __perf_cs_etmv4_magic) {
> > +                                       k++;
> > +                                       break;
> > +                               }
> > +                       }
> > +
> >                         /* The traceID is our handle */
> >                         idx = metadata[j][CS_ETMV4_TRCTRACEIDR];
> > -                       i += CS_ETMV4_PRIV_MAX;
> > +                       i += k;
> >                 }
> >
> >                 /* Get an RB node for this CPU */
> 
> That would be a spot fix for the read /copy case, but will not fix the
> print routine which will still bail out on older versions of the
> format. (when using perf report --dump).
> 
> The "self describing" format I have been looking at will add an
> NR_PARAMS value to the common block in the CPU metadata parameter
> list, increment the header version to '1' and update the format writer
> to use the version 1 format while having the reader understand both v0
> and v1 formats.
> 
> i..e in cs-etm.h perf I add:
> /*
>  * Update the version for new format.
>  *
>  * New version 1 format adds a param count to the per cpu metadata.
>  * This allows easy adding of new metadata parameters.
>  * Requires that new params always added after current ones.
>  * Also allows client reader to handle file versions that are different by
>  * checking the number of params in the file vs the number expected.
>  */
> #define CS_HEADER_CURRENT_VERSION 1
> 
> /* Beginning of header common to both ETMv3 and V4 */
> enum {
>     CS_ETM_MAGIC,
>     CS_ETM_CPU,
>     CS_ETM_NR_PARAMS, /* number of parameters to follow in this block */
> };
> 
> where in verison 1, NR_PARAMS indicates the total number of params
> that follow - so adding new parameters can be added to the metadata
> enums and the tool will automatically adjust, and will handle v0
> files, plus older and newer files that have differing numbers of
> parameters, as long as the parameters are only ever added to the end
> of the list.
> 
> I have been working on a patch for this today, which took a little
> longer than expected as it was a little more complex than expected
> (the printing routines in for the --dump command!).
> 
> I will post this tomorrow when tested - and if we agree it works it
> could be rolled into your set - it would make adding the PID parameter
> easier, and ensure that this new format is available for the upcoming
> developments.

Thanks for the info.  I will look at your patch and see how to fit
with it.

Thanks,
Leo