Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e890feb0-9f90-c0f2-e5f0-10ed5dbe6d81@arm.com>
Date:   Tue, 22 Mar 2022 10:43:26 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH 00/10] coresight: Add new API to allocate trace source ID
 values
Content-Language: en-US
To:     Mike Leach <mike.leach@linaro.org>, coresight@lists.linaro.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Cc:     mathieu.poirier@linaro.org, peterz@infradead.org, mingo@redhat.com,
        acme@kernel.org, linux-perf-users@vger.kernel.org,
        leo.yan@linaro.org, James Clark <james.clark@arm.com>
References: <20220308205000.27646-1-mike.leach@linaro.org>
From:   Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com>
In-Reply-To: <20220308205000.27646-1-mike.leach@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

+ Cc: James Clark

Hi Mike,

On 08/03/2022 20:49, Mike Leach wrote:
> The current method for allocating trace source ID values to sources is
> to use a fixed algorithm for CPU based sources of (cpu_num * 2 + 0x10).
> The STM is allocated ID 0x1.
> 
> This fixed algorithm is used in both the CoreSight driver code, and by
> perf when writing the trace metadata in the AUXTRACE_INFO record.
> 
> The method needs replacing as currently:-
> 1. It is inefficient in using available IDs.
> 2. Does not scale to larger systems with many cores and the algorithm
> has no limits so will generate invalid trace IDs for cpu number > 44.

Thanks for addressing this issue.

> 
> Additionally requirements to allocate additional system IDs on some
> systems have been seen.
> 
> This patch set  introduces an API that allows the allocation of trace IDs
> in a dynamic manner.
> 
> Architecturally reserved IDs are never allocated, and the system is
> limited to allocating only valid IDs.
> 
> Each of the current trace sources ETM3.x, ETM4.x and STM is updated to use
> the new API.
> 
> perf handling is changed so that the ID associated with the CPU is read
> from sysfs. The ID allocator is notified when perf events start and stop
> so CPU based IDs are kept constant throughout any perf session.
> 
> For the ETMx.x devices IDs are allocated on certain events
> a) When using sysfs, an ID will be allocated on hardware enable, and freed
> when the sysfs reset is written.
> b) When using perf, ID is allocated on hardware enable, and freed on
> hardware disable.
> 
> For both cases the ID is allocated when sysfs is read to get the current
> trace ID. This ensures that consistent decode metadata can be extracted
> from the system where this read occurs before device enable.


> 
> Note: This patchset breaks backward compatibility for perf record.
> Because the method for generating the AUXTRACE_INFO meta data has
> changed, using an older perf record will result in metadata that
> does not match the trace IDs used in the recorded trace data.
> This mismatch will cause subsequent decode to fail. Older versions of
> perf will still be able to decode data generated by the updated system.

I have some concerns over this and the future plans for the dynamic
allocation per sink. i.e., we are breaking/modifying the perf now to
accommodate the dynamic nature of the trace id of a given CPU/ETM.
The proposed approach of exposing this via sysfs may (am not sure if
this would be the case) break for the trace-id per sink change, as a
sink could assign different trace-id for a CPU depending.

So, instead if we make the trace-id available in the perf (say, an new
record format, PERF_RECORD_CS_ETM_TRACEID ?) record, we can rely on the
new packet for the trace-id, irrespective of how that is allocated and
remove the locking/linking of the trace-id with that of the sysfs. This
is not something that exists today. (Ideally it would have been nice to
have some additional fields in RECORD_AUXINFO, but we don't. Instead of
breaking/extending that, we could add a new RECORD).

I believe the packet may need to be generated only once for a session
and that will also allow the flexibility of moving trace-id allocation
around (to a sink in the future).

Thoughts ?

Kind regards
Suzuki


> 
> 
> Applies to coresight/next [b54f53bc11a5]
> Tested on DB410c
> 
> Mike Leach (10):
>    coresight: trace-id: Add API to dynamically assign trace ID values
>    coresight: trace-id: Set up source trace ID map for system
>    coresight: stm: Update STM driver to use Trace ID api
>    coresight: etm4x: Use trace ID API to dynamically allocate trace ID
>    coresight: etm3x: Use trace ID API to allocate IDs
>    coresight: perf: traceid: Add perf notifiers for trace ID
>    perf: cs-etm: Update event to read trace ID from sysfs
>    coresight: Remove legacy Trace ID allocation mechanism
>    coresight: etmX.X: stm: Remove unused legacy source trace ID ops
>    coresight: trace-id: Add debug & test macros to trace id allocation
> 
>   drivers/hwtracing/coresight/Makefile          |   2 +-
>   drivers/hwtracing/coresight/coresight-core.c  |  64 ++---
>   .../hwtracing/coresight/coresight-etm-perf.c  |  16 +-
>   drivers/hwtracing/coresight/coresight-etm.h   |   3 +-
>   .../coresight/coresight-etm3x-core.c          |  93 ++++---
>   .../coresight/coresight-etm3x-sysfs.c         |  28 +-
>   .../coresight/coresight-etm4x-core.c          |  63 ++++-
>   .../coresight/coresight-etm4x-sysfs.c         |  32 ++-
>   drivers/hwtracing/coresight/coresight-etm4x.h |   3 +
>   drivers/hwtracing/coresight/coresight-priv.h  |   1 +
>   drivers/hwtracing/coresight/coresight-stm.c   |  49 +---
>   .../hwtracing/coresight/coresight-trace-id.c  | 255 ++++++++++++++++++
>   .../hwtracing/coresight/coresight-trace-id.h  |  69 +++++
>   include/linux/coresight-pmu.h                 |  12 -
>   include/linux/coresight.h                     |   3 -
>   tools/perf/arch/arm/util/cs-etm.c             |  12 +-
>   16 files changed, 530 insertions(+), 175 deletions(-)
>   create mode 100644 drivers/hwtracing/coresight/coresight-trace-id.c
>   create mode 100644 drivers/hwtracing/coresight/coresight-trace-id.h
>