Message-ID: <CAJ9a7VjqGbpPPeR3-PH5vYHNMwqnPLJ+Ouik017Qh717wFOJ0g@mail.gmail.com>
Date: Tue, 1 Apr 2025 13:50:52 +0100
From: Mike Leach <mike.leach@...aro.org>
To: Leo Yan <leo.yan@....com>
Cc: Suzuki K Poulose <suzuki.poulose@....com>, James Clark <james.clark@...aro.org>, 
	Jonathan Corbet <corbet@....net>, Alexander Shishkin <alexander.shishkin@...ux.intel.com>, 
	coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/6] coresight: perf: Support AUX trace pause and resume

Hi Leo,

On Tue, 11 Mar 2025 at 17:05, Leo Yan <leo.yan@....com> wrote:
>
> This commit supports AUX trace pause and resume in a perf session for
> Arm CoreSight.
>
> First, we need to decide which flag can indicate the CoreSight PMU event
> has started.  The 'event->hw.state' cannot be used for this purpose
> because its initial value and the value after hardware trace enabling
> are both 0.
>
> On the other hand, the context value 'ctxt->event_data' stores the ETM
> private info.  This pointer is valid only when the PMU event has been
> enabled. It is safe to permit AUX trace pause and resume operations only
> when it is not a NULL pointer.
>
> To achieve fine-grained control of the pause and resume, only the tracer
> is disabled and enabled.  This avoids the unnecessary complexity and
> latency caused by manipulating the entire link path.
>
> Signed-off-by: Leo Yan <leo.yan@....com>
> ---
>  .../hwtracing/coresight/coresight-etm-perf.c  | 45 ++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index f4cccd68e625..2dcf1809cb7f 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -365,6 +365,18 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>                         continue;
>                 }
>
> +               /*
> +                * If AUX pause feature is enabled but the ETM driver does not
> +                * support the operations, clear this CPU from the mask and
> +                * continue to next one.
> +                */
> +               if (event->attr.aux_start_paused &&
> +                   (!source_ops(csdev)->pause_perf || !source_ops(csdev)->resume_perf)) {
> +                       dev_err_once(&csdev->dev, "AUX pause is not supported.\n");
> +                       cpumask_clear_cpu(cpu, mask);
> +                       continue;
> +               }
> +
>                 /*
>                  * No sink provided - look for a default sink for all the ETMs,
>                  * where this event can be scheduled.
> @@ -450,6 +462,15 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>         goto out;
>  }
>
> +static int etm_event_resume(struct coresight_device *csdev,
> +                            struct etm_ctxt *ctxt)
> +{
> +       if (!ctxt->event_data)
> +               return 0;
> +
> +       return coresight_resume_source(csdev);
> +}
> +
>  static void etm_event_start(struct perf_event *event, int flags)
>  {
>         int cpu = smp_processor_id();
> @@ -463,6 +484,14 @@ static void etm_event_start(struct perf_event *event, int flags)
>         if (!csdev)
>                 goto fail;
>

Is it possible here that the first call to etm_event_start() also has
the PERF_EF_RESUME flag set?
If so it looks like we need to fall through and do a "normal" start to
get all the ctxt->event_data set up.

> +       if (flags & PERF_EF_RESUME) {
> +               if (etm_event_resume(csdev, ctxt) < 0) {
> +                       dev_err(&csdev->dev, "Failed to resume ETM event.\n");
> +                       goto fail;
> +               }
> +               return;
> +       }
> +
>         /* Have we messed up our tracking ? */
>         if (WARN_ON(ctxt->event_data))
>                 goto fail;
> @@ -545,6 +574,16 @@ static void etm_event_start(struct perf_event *event, int flags)
>         return;
>  }
>
> +static void etm_event_pause(struct coresight_device *csdev,
> +                           struct etm_ctxt *ctxt)
> +{
> +       if (!ctxt->event_data)
> +               return;
> +
> +       /* Stop tracer */
> +       coresight_pause_source(csdev);
> +}
> +
>  static void etm_event_stop(struct perf_event *event, int mode)
>  {
>         int cpu = smp_processor_id();
> @@ -555,6 +594,9 @@ static void etm_event_stop(struct perf_event *event, int mode)
>         struct etm_event_data *event_data;
>         struct coresight_path *path;
>
> +       if (mode & PERF_EF_PAUSE)
> +               return etm_event_pause(csdev, ctxt);
> +
>         /*
>          * If we still have access to the event_data via handle,
>          * confirm that we haven't messed up the tracking.
> @@ -899,7 +941,8 @@ int __init etm_perf_init(void)
>         int ret;
>
>         etm_pmu.capabilities            = (PERF_PMU_CAP_EXCLUSIVE |
> -                                          PERF_PMU_CAP_ITRACE);
> +                                          PERF_PMU_CAP_ITRACE |
> +                                          PERF_PMU_CAP_AUX_PAUSE);
>
>         etm_pmu.attr_groups             = etm_pmu_attr_groups;
>         etm_pmu.task_ctx_nr             = perf_sw_context;
> --
> 2.34.1
>

If the possible issue above is prevented by perf internals

Reviewed-by: Mike Leach <mike.leach@...aro.org>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Powered by blists - more mailing lists