Re: [PATCH 2/4] coresight: tmc-etr: Track perf handler.

From: Leo Yan <leo.yan@linaro.org>
To: Daniel Kiss <daniel.kiss@arm.com>
Cc: mathieu.poirier@linaro.org, suzuki.poulose@arm.com,
	mike.leach@linaro.org, coresight@lists.linaro.org,
	linux-arm-kernel@lists.infradead.org, denik@google.com,
	Branislav Rankov <Branislav.Rankov@arm.com>
Subject: Re: [PATCH 2/4] coresight: tmc-etr: Track perf handler.
Date: Mon, 26 Apr 2021 08:25:51 +0800	[thread overview]
Message-ID: <20210426002551.GA176271@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <20210423092038.GB377091@leoy-ThinkPad-X240s>

On Fri, Apr 23, 2021 at 05:20:38PM +0800, Leo Yan wrote:
> Hi Daniel,
> 
> On Wed, Apr 21, 2021 at 02:04:11PM +0200, Daniel Kiss wrote:
> 
> [...]
> 
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > index dd19d1d1c3b38..bf9f6311d8663 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > @@ -1511,6 +1511,12 @@ tmc_update_etr_buffer(struct coresight_device *csdev,
> >  		goto out;
> >  	}
> >  
> > +	/* Serve only the tracer with the right handler */
> > +	if (drvdata->perf_handle != handle) {
> > +		spin_unlock_irqrestore(&drvdata->spinlock, flags);
> > +		goto out;
> > +	}
> > +
> 
> I have concern for this change, Let's use the system-wide tracing as
> an example.
> 
> If a system have 4 CPUs, for the perf with system wide tracing, the
> tool maps the AUX ring buffers for four times, but the CoreSight
> driver only allocates pages once and maps these physical pages for
> four times to user space.  Therefore, the perf tool in the userspace
> manages 4 AUX ring buffers, every AUX ring buffer is served for one
> CPU.
> 
> The confusion between the CoreSight driver (in the kernel) and the
> perf tool (in the userspace) is: there actually has only one ring
> buffer for the enabled sink (let's say ETR), but there have four ring
> buffer control structures, the control structure is
> 'perf_event_mmap_page' which is resident in the first page for perf's
> general ring buffer (please note, this ring buffer is different from
> AUX ring buffer).
> 
> IIUC, this patch only allows the first CPU which enables coresight path
> to update the AUX ring buffer.  This can break the case:
> 
>   - Step 1: perf tool opens ETM event; we can use the command:
> 
>     # perf record -o ${perfdata} -e cs_etm/@tmc_etr0/ -a
>            -- dd if=/dev/zero of=/dev/null
> 
>   - Step 2: the profiled program "dd" is firstly schedued in CPU0, so
>     its "perf_handle" will be assigned to "drvdata->perf_handle";
> 
>   - Step 3: if the program "dd" is migrated to CPU1 and it never runs
>     on CPU0 afterwards, then this patch will prevent to update the AUX
>     ring buffer, due to the "drvdata->perf_handle" cannot match with
>     CPU1's handler.

Want to clarify, this case only happens with "snapshot" mode; With
Mathieu's reminding, "snapshot" mode is quite special: it creates AUX
ring buffer per CPU, but when enable the tracing, if without
specifying the option "-a" for system wide tracing, it only enables
ETM tracer for a CPU when the profiled program is scheduled on that CPU.

To avoid over complexsity, let's give this low priority and firstly
focus on the system-wide tracing case.

Thanks,
Leo

> On the other hand, I think we should change to always stick to the
> same "perf_output_handle" for all CPUs, thus it can allow all CPUs
> to use the same structure 'perf_event_mmap_page' for AUX ring buffer
> management.

> 
> [...]
> 
> Thanks,
> Leo

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