Re: [PATCH 2/3] perf: cs_etm: Use pid tracing explicitly instead of contextid

Leo Yan
2020-11-12T12:24:44Z

On Thu, Nov 12, 2020 at 10:54:23AM +0000, Suzuki Kuruppassery Poulose wrote:
> Hi Leo,
>
> Thanks for looking at the patch.

Welcome!

[...]

> > > + switch (pid_fmt) {
> > > + case (1ULL << ETM_OPT_CTXTID):
> > > + /*
> > > + * TRCIDR2.CIDSIZE, bit [9-5], indicates whether contextID tracing
> > > + * is supported:
> > > + * 0b00000 Context ID tracing is not supported.
> > > + * 0b00100 Maximum of 32-bit Context ID size.
> > > + * All other values are reserved.
> > > + */
> > > + val = BMVAL(val, 5, 9);
> > > + if (!val || val != 0x4) {
> > > + err = -EINVAL;
> > > + goto out;
> > > + }
> > > + break;
> > > + case (1ULL << ETM_OPT_CTXTID_IN_VMID):
> > > + /*
> > > + * TRCIDR2.VMIDOPT[30:29] != 0 and
> > > + * TRCIDR2.VMIDSIZE[14:10] == 0b00100 (32bit virtual context id size)
> > > + */
> > > + if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) < 4) {
> >
> > The comment is not alignment with the code. Based on the comment, the
> > code should be:
> >
> > if (!BMVAL(val, 29, 30) || BMVAL(val, 10, 14) != 4) {
> >
>
> The reasoning is any value > 4, would imply a size > 32. This is really making
> it future proof. I could restrict this to 4 now if you insist.

So here you want to filter out the cases for 16-bit and 8-bit virtual
context ID size, but want to leave the possibility for support size >
32-bit in future.

It's fine to keep current code, but it's better to add description in
the comment, otherwise, this might be difficult for maintenance.

> > > +#define ETM_SET_OPT_PID (1 << 0)
> > > +#define ETM_SET_OPT_TS (1 << 1)
> > > +#define ETM_SET_OPT_MASK (ETM_SET_OPT_PID | ETM_SET_OPT_TS)
> > > +
> > > static int cs_etm_set_option(struct auxtrace_record *itr,
> > > struct evsel *evsel, u32 option)
> > > {
> > > @@ -169,17 +196,17 @@ static int cs_etm_set_option(struct auxtrace_record *itr,
> > > !cpu_map__has(online_cpus, i))
> > > continue;
> > > - if (option & ETM_OPT_CTXTID) {
> > > - err = cs_etm_set_context_id(itr, evsel, i);
> > > + if (option & ETM_SET_OPT_PID) {
> > > + err = cs_etm_set_pid(itr, evsel, i);
> >
> > I don't understand what's the reason for introducing the new macros
> > "ETM_SET_OPT_XXX", seems to me the old macros still can be used at
> > here. Could you help explian for this?
>
> Sure, with the change now, we either set ETM_OPT_CTXTID or ETM_OPT_CTXTID_IN_VMID
> in the config. So, using the flag ETM_OPT_CTXTID to indicate the same is going to
> be confusing. This makes it explicit and uses a separate flag. Or the in otherways
> it is really making clear that we want to set PID tracing and de-coupling it from
> "CONTEXTID" as it is not the correct config anymore and has to be determined at
> runtime. I could make it explicit in a comment.

Makes sense for me; thanks for the explanation.

Leo