[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM9d7chfXH0ynfT_PSPyjhE8ATa=fV-kbx_csgeQrWiv+1EiiQ@mail.gmail.com>
Date: Fri, 22 Mar 2024 18:22:36 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ben Gainey <ben.gainey@....com>
Cc: peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
james.clark@....com, mark.rutland@....com, alexander.shishkin@...ux.intel.com,
jolsa@...nel.org, irogers@...gle.com, adrian.hunter@...el.com,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 0/4] perf: Support PERF_SAMPLE_READ with inherit_stat
On Fri, Mar 22, 2024 at 9:42 AM Ben Gainey <ben.gainey@....com> wrote:
>
> This change allows events to use PERF_SAMPLE READ with inherit so long
> as both inherit_stat and PERF_SAMPLE_TID are set.
>
> Currently it is not possible to use PERF_SAMPLE_READ with inherit. This
> restriction assumes the user is interested in collecting aggregate
> statistics as per `perf stat`. It prevents a user from collecting
> per-thread samples using counter groups from a multi-threaded or
> multi-process application, as with `perf record -e '{....}:S'`. Instead
> users must use system-wide mode, or forgo the ability to sample counter
> groups. System-wide mode is often problematic as it requires specific
> permissions (no CAP_PERFMON / root access), or may lead to capture of
> significant amounts of extra data from other processes running on the
> system.
>
> Perf already supports the ability to collect per-thread counts with
> `inherit` via the `inherit_stat` flag. This patch changes
I'm not sure about this part. IIUC inherit and inherit_stat is not for
per-thread counts, it only supports per-process (including children)
events.
> `perf_event_alloc` relaxing the restriction to combine `inherit` with
> `PERF_SAMPLE_READ` so that the combination will be allowed so long as
> `inherit_stat` and `PERF_SAMPLE_TID` are enabled.
Anyway, does it really need 'inherit_stat'? I think it's only for
counting use cases (e.g. 'perf stat') not for sampling.
Also technically, it can have PERF_SAMPLE_STREAM_ID instead
of PERF_SAMPLE_TID to distinguish the counter values.
>
> In this configuration stream ids (such as may appear in the read_format
> field of a PERF_RECORD_SAMPLE) are no longer globally unique, rather
> the pair of (stream id, tid) uniquely identify each event. Tools that
> rely on this, for example to calculate a delta between samples, would
> need updating to take this into account. Previously valid event
> configurations (system-wide, no-inherit and so on) where each stream id
> is the identifier are unaffected.
I think you meant PERF_SAMPLE_ID not PERF_SAMPLE_STREAM_ID.
IIUC the stream id is already unique.
Thanks,
Namhyung
>
>
> Changes since v3:
> - Cleaned up perf test data changes incorrectly included into this
> series from elsewhere.
>
> Changes since v2:
> - Rebase on v6.8
> - Respond to James Clarke's feedback; fixup some typos and move some
> repeated checks into a helper macro.
> - Cleaned up checkpatch lints.
> - Updated perf test; fixed evsel handling so that existing tests pass
> and added new tests to cover the new behaviour.
>
> Changes since v1:
> - Rebase on v6.8-rc1
> - Fixed value written into sample after child exists.
> - Modified handling of switch-out so that context with these events
> take the slow path, so that the per-event/per-thread PMU state is
> correctly switched.
> - Modified perf tools to support this mode of operation.
>
> Ben Gainey (4):
> perf: Support PERF_SAMPLE_READ with inherit_stat
> tools/perf: Track where perf_sample_ids need per-thread periods
> tools/perf: Correctly calculate sample period for inherited
> SAMPLE_READ values
> tools/perf: Allow inherit + inherit_stat + PERF_SAMPLE_READ when
> opening events
>
> include/linux/perf_event.h | 1 +
> kernel/events/core.c | 62 ++++++++++----
> tools/lib/perf/evlist.c | 1 +
> tools/lib/perf/evsel.c | 48 +++++++++++
> tools/lib/perf/include/internal/evsel.h | 55 ++++++++++++-
> .../test-record-group-sampling-inherit-stat | 62 ++++++++++++++
> tools/perf/util/evsel.c | 82 ++++++++++++++++++-
> tools/perf/util/evsel.h | 1 +
> tools/perf/util/session.c | 11 ++-
> 9 files changed, 301 insertions(+), 22 deletions(-)
> create mode 100644 tools/perf/tests/attr/test-record-group-sampling-inherit-stat
>
> --
> 2.44.0
>
Powered by blists - more mailing lists