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

From: Leo Yan <leo.yan@linaro.org>
To: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: al.grant@arm.com, mathieu.poirier@linaro.org,
	anshuman.khandual@arm.com, coresight@lists.linaro.org,
	linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org
Subject: Re: [PATCH 2/3] perf: cs_etm: Use pid tracing explicitly instead of contextid
Date: Thu, 12 Nov 2020 20:24:10 +0800	[thread overview]
Message-ID: <20201112122410.GD17274@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <ea90e2a9-c467-30d4-735e-d31695980b6e@arm.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel