Date:   Fri, 8 Oct 2021 17:12:04 -0700
From:   Namhyung Kim <namhyung@...nel.org>
To:     German Gomez <german.gomez@....com>
Cc:     Leo Yan <leo.yan@...aro.org>, James Clark <james.clark@....com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>, Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Ian Rogers <irogers@...gle.com>,
        Stephane Eranian <eranian@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [RFC] perf arm-spe: Track task context switch for cpu-mode events

Hi German,

On Fri, Oct 8, 2021 at 4:08 AM German Gomez <german.gomez@....com> wrote:
>
> Hi Leo, Namhyung,
>
> On 06/10/2021 17:09, Namhyung Kim wrote:
> > Hi Leo and German,
> >
> > [...]
> >
> > I think it'd be better to check it in perf record and not set
> > evsel->core.attr.context_switch if possible.
> >
> > Or it can ignore the context switch once it sees a context packet.
> >
> >> Here should note one thing is the perf tool needs to have knowledge to
> >> decide if the bit 3 'CX' (macro 'SYS_PMSCR_EL1_CX_SHIFT' in kernel) has
> >> been set in register PMSCR or not.  AFAIK, Arm SPE driver doesn't
> >> expose any interface (or config) to userspace for the context tracing,
> >> so one method is to add an extra config in Arm SPE driver for this
> >> bit, e.g. 'ATTR_CFG_FLD_cx_enable_CFG' can be added in Arm SPE driver.
> >>
> >> Alternatively, rather than adding new config, I am just wandering we
> >> simply use two flags in perf's decoding: 'use_switch_event_for_pid' and
> >> 'use_ctx_packet_for_pid', the first variable will be set if detects
> >> the tracing is userspace only, the second varaible will be set when
> >> detects the hardware tracing containing context packet.  So if the
> >> variable 'use_ctx_packet_for_pid' has been set, then the decoder will
> >> always use context packet for sample's PID, otherwise, it falls back
> >> to check 'use_switch_event_for_pid' and set sample PID based on switch
> >> events.
> >>
> >> If have any other idea, please feel free bring up.
> > If it's just kernel config, we can check /proc/config.gz or
> > /boot/config-$(uname -r).  When it knows for sure it can just use
> > the context packet, otherwise it needs the context switch.
> >
> > Thanks,
> > Namhyung
>
> Please correct me if I'm wrong, after disabling the PID_IN_CONTEXTIDR
> feature in the kernel, I don't see any context packets in the auxtraces.
> I think after applying the patch from [1], it should be sufficient to
> determine if pid tracing should fall back to use --switch-events when
> context_id from that patch has a value of -1.
>
> If the patch at the end of this message is applied on top of Namhyuna's
> and [1], I think it can work. Also, if the pmu driver is patched to
> disable the 'CX' bit when the pid is not in the root namespace [2]
> (unfortunately I haven't been able to set up an environment to properly
> test Leo's patch yet) tracing could also fall back to context-switch for
> userspace tracing. What do you think?

I think we should use context-switch even for kernel samples, but
only if the context packets are not available.

Thanks,
Namhyung

>
> Thanks,
> German
>
> [1] https://www.spinics.net/lists/linux-perf-users/msg12543.html
> [2] https://lore.kernel.org/lkml/20210916135418.GA383600@leoy-ThinkPad-X240s/
>
> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> index 708323d..e224665 100644
> --- a/tools/perf/util/arm-spe.c
> +++ b/tools/perf/util/arm-spe.c
> @@ -71,6 +71,12 @@
>      u64                kernel_start;
>
>      unsigned long            num_events;
> +
> +    /*
> +     * Used for PID tracing.
> +     */
> +    u8                use_context_id_pkt;
> +    u8                use_context_switch_event;
>  };
>
>  struct arm_spe_queue {
> @@ -586,13 +592,30 @@
>      return timeless_decoding;
>  }
>
> +static bool arm_spe__is_exclude_kernel(struct arm_spe *spe) {
> +    struct evsel *evsel;
> +    struct evlist *evlist = spe->session->evlist;
> +
> +    evlist__for_each_entry(evlist, evsel) {
> +        if (evsel->core.attr.type == spe->pmu_type && evsel->core.attr.exclude_kernel)
> +            return true;
> +    }
> +
> +    return false;
> +}
> +
>  static void arm_spe_set_pid_tid_cpu(struct arm_spe *spe,
>                      struct auxtrace_queue *queue)
>  {
>      struct arm_spe_queue *speq = queue->priv;
> -    pid_t tid;
> +    pid_t tid = -1;
>
> -    tid = machine__get_current_tid(spe->machine, speq->cpu);
> +    if (spe->use_context_id_pkt)
> +        tid = speq->decoder->record.context_id;
> +
> +    if (tid == -1 && spe->use_context_switch_event)
> +        tid = machine__get_current_tid(spe->machine, speq->cpu);
> +
>      if (tid != -1) {
>          speq->tid = tid;
>          thread__zput(speq->thread);
> @@ -1084,6 +1107,15 @@
>      spe->timeless_decoding = arm_spe__is_timeless_decoding(spe);
>
>      /*
> +     * Always try to use context packet by default for pid tracing.
> +     *
> +     * If it's not enabled in the pmu driver, it will always have a value of -1 and we can try
> +     * to fall back to using context-switch events instead.
> +     */
> +    spe->use_context_id_pkt = true;
> +    spe->use_context_switch_event = arm_spe__is_exclude_kernel(spe);
> +
> +    /*
>       * The synthesized event PERF_RECORD_TIME_CONV has been handled ahead
>       * and the parameters for hardware clock are stored in the session
>       * context.  Passes these parameters to the struct perf_tsc_conversion
> @@ -1141,4 +1173,4 @@
>  err_free:
>      free(spe);
>      return err;
> -}
> +}
> \ No newline at end of file

Powered by blists - more mailing lists