| Message ID | 20250310104919.58816-6-leo.yan@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Arm CoreSight: Support AUX pause and resume | expand |
Hi Leo On 10/03/2025 10:49, Leo Yan wrote: > Add an attribute for updating buffer when the AUX trace is paused. And > populate the value to the 'update_buf_on_pause' flag during the AUX > setting up. Do we need this attribute in the uAPI ? Could we do this by default for sinks without interrupt ? This definitely improves the quality of trace collected for such sinks and the driver can transparently do this. Cheers Suzuki > > If the AUX pause operation is attached to a PMU counter, when the > counter is overflow and if the PMU interrupt in an NMI, then AUX pause > operation will be triggered in the NMI context. On the other hand, the > per CPU sink has its own interrupt handling. Thus, there will be a race > condition between the updating buffer in NMI and sink's interrupt > handler. > > To avoid the race condition, this commit disallows updating buffer on > AUX pause for the per CPU sink. Currently, this is only applied for > TRBE. > > Signed-off-by: Leo Yan <leo.yan@arm.com> > --- > .../hwtracing/coresight/coresight-etm-perf.c | 20 +++++++++++++++++++ > .../hwtracing/coresight/coresight-etm-perf.h | 2 ++ > include/linux/coresight-pmu.h | 1 + > 3 files changed, 23 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c > index 29d52386ffbb..d759663a1f7d 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c > @@ -62,6 +62,8 @@ PMU_FORMAT_ATTR(contextid1, "config:" __stringify(ETM_OPT_CTXTID)); > PMU_FORMAT_ATTR(contextid2, "config:" __stringify(ETM_OPT_CTXTID2)); > PMU_FORMAT_ATTR(timestamp, "config:" __stringify(ETM_OPT_TS)); > PMU_FORMAT_ATTR(retstack, "config:" __stringify(ETM_OPT_RETSTK)); > +PMU_FORMAT_ATTR(update_buf_on_pause, > + "config:" __stringify(ETM_OPT_UPDATE_BUF_ON_PAUSE)); > /* preset - if sink ID is used as a configuration selector */ > PMU_FORMAT_ATTR(preset, "config:0-3"); > /* Sink ID - same for all ETMs */ > @@ -103,6 +105,7 @@ static struct attribute *etm_config_formats_attr[] = { > &format_attr_configid.attr, > &format_attr_branch_broadcast.attr, > &format_attr_cc_threshold.attr, > + &format_attr_update_buf_on_pause.attr, > NULL, > }; > > @@ -434,6 +437,23 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, > if (!sink) > goto err; > > + /* Populate the flag for updating buffer on AUX pause */ > + event_data->update_buf_on_pause = > + !!(event->attr.config & BIT(ETM_OPT_UPDATE_BUF_ON_PAUSE)); > + > + if (event_data->update_buf_on_pause) { > + /* > + * The per CPU sink has own interrupt handling, it might have > + * race condition with updating buffer on AUX trace pause if > + * it is invoked from NMI. To avoid the race condition, > + * disallows updating buffer for the per CPU sink case. > + */ > + if (coresight_is_percpu_sink(sink)) { > + dev_err(&sink->dev, "update_buf_on_pause is not permitted.\n"); > + goto err; > + } > + } > + > /* If we don't have any CPUs ready for tracing, abort */ > cpu = cpumask_first(mask); > if (cpu >= nr_cpu_ids) > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h > index 744531158d6b..52b9385f8c11 100644 > --- a/drivers/hwtracing/coresight/coresight-etm-perf.h > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h > @@ -51,6 +51,7 @@ struct etm_filters { > * @aux_hwid_done: Whether a CPU has emitted the TraceID packet or not. > * @snk_config: The sink configuration. > * @cfg_hash: The hash id of any coresight config selected. > + * @update_buf_on_pause: The flag to indicate updating buffer on AUX pause. > * @path: An array of path, each slot for one CPU. > */ > struct etm_event_data { > @@ -59,6 +60,7 @@ struct etm_event_data { > cpumask_t aux_hwid_done; > void *snk_config; > u32 cfg_hash; > + bool update_buf_on_pause; > struct list_head * __percpu *path; > }; > > diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h > index 89b0ac0014b0..04147e30c2f2 100644 > --- a/include/linux/coresight-pmu.h > +++ b/include/linux/coresight-pmu.h > @@ -35,6 +35,7 @@ > #define ETM_OPT_CTXTID2 15 > #define ETM_OPT_TS 28 > #define ETM_OPT_RETSTK 29 > +#define ETM_OPT_UPDATE_BUF_ON_PAUSE 30 > > /* ETMv4 CONFIGR programming bits for the ETM OPTs */ > #define ETM4_CFG_BIT_BB 3
On Mon, Mar 10, 2025 at 01:29:26PM +0000, Suzuki Kuruppassery Poulose wrote: > Hi Leo > > On 10/03/2025 10:49, Leo Yan wrote: > > Add an attribute for updating buffer when the AUX trace is paused. And > > populate the value to the 'update_buf_on_pause' flag during the AUX > > setting up. > > Do we need this attribute in the uAPI ? This uAPI allows users to perform AUX pause and resume without the long latency caused by copying hardware trace data. E.g., a user can specify a large AUX buffer size using option "-m,128M". If the buffer is considered large enough to accommodate hardware trace data for a small program, the 'update_buf_on_pause' flag can be set to false, the copying will be deferred until the end of the perf session. I am bias to keep this uAPI. If you prefer to remove it, I am also fine with that. > Could we do this by default for > sinks without interrupt ? This definitely improves the quality of trace > collected for such sinks and the driver can transparently do this. How about we dynamically set the default flag in the Perf tool? - If users set explictly the 'update_buf_on_pause' flag, then the setting will be respected. - If users don't set the flag, perf tool detects it is TRBE sinks, then it can set 'update_buf_on_pause' flag as false. - If users don't set the flag, perf tool detects it is ETF/ETB/ETR sinks, it sets the flag as true. Thanks, Leo
On 10/03/2025 15:50, Leo Yan wrote: > On Mon, Mar 10, 2025 at 01:29:26PM +0000, Suzuki Kuruppassery Poulose wrote: >> Hi Leo >> >> On 10/03/2025 10:49, Leo Yan wrote: >>> Add an attribute for updating buffer when the AUX trace is paused. And >>> populate the value to the 'update_buf_on_pause' flag during the AUX >>> setting up. >> >> Do we need this attribute in the uAPI ? > > This uAPI allows users to perform AUX pause and resume without the long > latency caused by copying hardware trace data. > > E.g., a user can specify a large AUX buffer size using option "-m,128M". > If the buffer is considered large enough to accommodate hardware trace > data for a small program, the 'update_buf_on_pause' flag can be set to > false, the copying will be deferred until the end of the perf session. > > I am bias to keep this uAPI. If you prefer to remove it, I am also > fine with that. > >> Could we do this by default for >> sinks without interrupt ? This definitely improves the quality of trace >> collected for such sinks and the driver can transparently do this. > > How about we dynamically set the default flag in the Perf tool? > > - If users set explictly the 'update_buf_on_pause' flag, then the > setting will be respected. > - If users don't set the flag, perf tool detects it is TRBE sinks, > then it can set 'update_buf_on_pause' flag as false. Not really possible. There could be systems with mixed sinks. e.g. TRBE for some CPU and ETR for others (say due to a non-functioning TRBE). > - If users don't set the flag, perf tool detects it is ETF/ETB/ETR > sinks, it sets the flag as true. And in the cases above, perf event cannot run on all the CPUs, because some sinks don't support it. Why do we need a flag, when the effect is not user (read, perf decoder) visible and at the same time improves some scenarios (read non-TRBE cases) ? I would say, let the driver always update on pause, depending on the sink. Suzuki > > Thanks, > Leo
On Mon, Mar 10, 2025 at 04:37:44PM +0000, Suzuki Kuruppassery Poulose wrote: [...] > > How about we dynamically set the default flag in the Perf tool? > > > > - If users set explictly the 'update_buf_on_pause' flag, then the > > setting will be respected. > > - If users don't set the flag, perf tool detects it is TRBE sinks, > > then it can set 'update_buf_on_pause' flag as false. > > Not really possible. There could be systems with mixed sinks. e.g. TRBE > for some CPU and ETR for others (say due to a non-functioning TRBE). > > > - If users don't set the flag, perf tool detects it is ETF/ETB/ETR > > sinks, it sets the flag as true. > > And in the cases above, perf event cannot run on all the CPUs, because > some sinks don't support it. > > Why do we need a flag, when the effect is not user (read, perf decoder) > visible and at the same time improves some scenarios (read non-TRBE cases) > ? Indeed in this case the flag is redundant. > I would say, let the driver always update on pause, depending on the > sink. It is fine for me. I will move towards this direction. Thanks, Leo
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 29d52386ffbb..d759663a1f7d 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -62,6 +62,8 @@ PMU_FORMAT_ATTR(contextid1, "config:" __stringify(ETM_OPT_CTXTID)); PMU_FORMAT_ATTR(contextid2, "config:" __stringify(ETM_OPT_CTXTID2)); PMU_FORMAT_ATTR(timestamp, "config:" __stringify(ETM_OPT_TS)); PMU_FORMAT_ATTR(retstack, "config:" __stringify(ETM_OPT_RETSTK)); +PMU_FORMAT_ATTR(update_buf_on_pause, + "config:" __stringify(ETM_OPT_UPDATE_BUF_ON_PAUSE)); /* preset - if sink ID is used as a configuration selector */ PMU_FORMAT_ATTR(preset, "config:0-3"); /* Sink ID - same for all ETMs */ @@ -103,6 +105,7 @@ static struct attribute *etm_config_formats_attr[] = { &format_attr_configid.attr, &format_attr_branch_broadcast.attr, &format_attr_cc_threshold.attr, + &format_attr_update_buf_on_pause.attr, NULL, }; @@ -434,6 +437,23 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, if (!sink) goto err; + /* Populate the flag for updating buffer on AUX pause */ + event_data->update_buf_on_pause = + !!(event->attr.config & BIT(ETM_OPT_UPDATE_BUF_ON_PAUSE)); + + if (event_data->update_buf_on_pause) { + /* + * The per CPU sink has own interrupt handling, it might have + * race condition with updating buffer on AUX trace pause if + * it is invoked from NMI. To avoid the race condition, + * disallows updating buffer for the per CPU sink case. + */ + if (coresight_is_percpu_sink(sink)) { + dev_err(&sink->dev, "update_buf_on_pause is not permitted.\n"); + goto err; + } + } + /* If we don't have any CPUs ready for tracing, abort */ cpu = cpumask_first(mask); if (cpu >= nr_cpu_ids) diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 744531158d6b..52b9385f8c11 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -51,6 +51,7 @@ struct etm_filters { * @aux_hwid_done: Whether a CPU has emitted the TraceID packet or not. * @snk_config: The sink configuration. * @cfg_hash: The hash id of any coresight config selected. + * @update_buf_on_pause: The flag to indicate updating buffer on AUX pause. * @path: An array of path, each slot for one CPU. */ struct etm_event_data { @@ -59,6 +60,7 @@ struct etm_event_data { cpumask_t aux_hwid_done; void *snk_config; u32 cfg_hash; + bool update_buf_on_pause; struct list_head * __percpu *path; }; diff --git a/include/linux/coresight-pmu.h b/include/linux/coresight-pmu.h index 89b0ac0014b0..04147e30c2f2 100644 --- a/include/linux/coresight-pmu.h +++ b/include/linux/coresight-pmu.h @@ -35,6 +35,7 @@ #define ETM_OPT_CTXTID2 15 #define ETM_OPT_TS 28 #define ETM_OPT_RETSTK 29 +#define ETM_OPT_UPDATE_BUF_ON_PAUSE 30 /* ETMv4 CONFIGR programming bits for the ETM OPTs */ #define ETM4_CFG_BIT_BB 3
Add an attribute for updating buffer when the AUX trace is paused. And populate the value to the 'update_buf_on_pause' flag during the AUX setting up. If the AUX pause operation is attached to a PMU counter, when the counter is overflow and if the PMU interrupt in an NMI, then AUX pause operation will be triggered in the NMI context. On the other hand, the per CPU sink has its own interrupt handling. Thus, there will be a race condition between the updating buffer in NMI and sink's interrupt handler. To avoid the race condition, this commit disallows updating buffer on AUX pause for the per CPU sink. Currently, this is only applied for TRBE. Signed-off-by: Leo Yan <leo.yan@arm.com> --- .../hwtracing/coresight/coresight-etm-perf.c | 20 +++++++++++++++++++ .../hwtracing/coresight/coresight-etm-perf.h | 2 ++ include/linux/coresight-pmu.h | 1 + 3 files changed, 23 insertions(+)