| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aRwWGPs0IQoQesoo@google.com>
Date: Mon, 17 Nov 2025 22:45:44 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
"Dr. David Alan Gilbert" <linux@...blig.org>,
Yang Li <yang.lee@...ux.alibaba.com>,
James Clark <james.clark@...aro.org>,
Thomas Falcon <thomas.falcon@...el.com>,
Thomas Richter <tmricht@...ux.ibm.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
Andi Kleen <ak@...ux.intel.com>,
Dapeng Mi <dapeng1.mi@...ux.intel.com>
Subject: Re: [PATCH v4 04/10] perf stat-shadow: Read tool events directly
On Mon, Nov 17, 2025 at 08:36:34PM -0800, Ian Rogers wrote:
> On Mon, Nov 17, 2025 at 6:30 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > On Thu, Nov 13, 2025 at 10:05:10AM -0800, Ian Rogers wrote:
> > > When reading time values for metrics don't use the globals updated in
> > > builtin-stat, just read the events as regular events. The only
> > > exception is for time events where nanoseconds need converting to
> > > seconds as metrics assume time metrics are in seconds.
> > >
> > > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > > ---
[SNIP]
> > > - case TOOL_PMU__EVENT_SYSTEM_TIME:
> > > - stats = &ru_stats.ru_stime_usec_stat;
> > > - scale = 1e-6;
> > > - break;
> >
> > I think this was broken as it seems to be converted to nsec in the
> > update_rusage_stats().
>
> The global utime/stime is in usecs as in the name. In the tool_pmu it
> is converted to nanoseconds, for consistency with duration_time. In
> the new code the counters are all in nanoseconds and so the same
> scaling factor can be applied.
What do you mean by global utime/stime?
>
> > > - case TOOL_PMU__EVENT_NONE:
> > > - pr_err("Invalid tool event 'none'");
> > > - abort();
> > > - case TOOL_PMU__EVENT_MAX:
> > > - pr_err("Invalid tool event 'max'");
> > > - abort();
> > > - case TOOL_PMU__EVENT_HAS_PMEM:
> > > - case TOOL_PMU__EVENT_NUM_CORES:
> > > - case TOOL_PMU__EVENT_NUM_CPUS:
> > > - case TOOL_PMU__EVENT_NUM_CPUS_ONLINE:
> > > - case TOOL_PMU__EVENT_NUM_DIES:
> > > - case TOOL_PMU__EVENT_NUM_PACKAGES:
> > > - case TOOL_PMU__EVENT_SLOTS:
> > > - case TOOL_PMU__EVENT_SMT_ON:
> > > - case TOOL_PMU__EVENT_SYSTEM_TSC_FREQ:
> > > - case TOOL_PMU__EVENT_CORE_WIDE:
> > > - case TOOL_PMU__EVENT_TARGET_CPU:
> > > - default:
> > > - pr_err("Unexpected tool event '%s'", evsel__name(metric_events[i]));
> > > - abort();
> > > }
> > > - val = avg_stats(stats) * scale;
> > > - source_count = 1;
> > > - } else {
> > > - struct perf_stat_evsel *ps = metric_events[i]->stats;
> > > - struct perf_stat_aggr *aggr;
> > > -
> > > + }
> > > + /* Time events are always on CPU0, the first aggregation index. */
> > > + aggr = &ps->aggr[is_tool_time ? tool_aggr_idx : aggr_idx];
> > > + if (!aggr || !metric_events[i]->supported) {
> > > /*
> > > - * If there are multiple uncore PMUs and we're not
> > > - * reading the leader's stats, determine the stats for
> > > - * the appropriate uncore PMU.
> > > + * Not supported events will have a count of 0, which
> > > + * can be confusing in a metric. Explicitly set the
> > > + * value to NAN. Not counted events (enable time of 0)
> > > + * are read as 0.
> > > */
> > > - if (evsel && evsel->metric_leader &&
> > > - evsel->pmu != evsel->metric_leader->pmu &&
> > > - mexp->metric_events[i]->pmu == evsel->metric_leader->pmu) {
> > > - struct evsel *pos;
> > > -
> > > - evlist__for_each_entry(evsel->evlist, pos) {
> > > - if (pos->pmu != evsel->pmu)
> > > - continue;
> > > - if (pos->metric_leader != mexp->metric_events[i])
> > > - continue;
> > > - ps = pos->stats;
> > > - source_count = 1;
> > > - break;
> > > - }
> > > - }
> > > - aggr = &ps->aggr[aggr_idx];
> > > - if (!aggr)
> > > - break;
> > > -
> > > - if (!metric_events[i]->supported) {
> > > - /*
> > > - * Not supported events will have a count of 0,
> > > - * which can be confusing in a
> > > - * metric. Explicitly set the value to NAN. Not
> > > - * counted events (enable time of 0) are read as
> > > - * 0.
> > > - */
> > > - val = NAN;
> > > - source_count = 0;
> > > - } else {
> > > - val = aggr->counts.val;
> > > - if (!source_count)
> > > - source_count = evsel__source_count(metric_events[i]);
> > > - }
> > > + val = NAN;
> > > + source_count = 0;
> > > + } else {
> > > + val = aggr->counts.val;
> > > + if (is_tool_time)
> > > + val *= 1e-9; /* Convert time event nanoseconds to seconds. */
> >
> > And this code treats all time events are in nsec now.
>
> That's correct: user_time, system_time and duration_time are all
> counters that give a count in nanoseconds. However, in the metrics, I
> guess as the counts are doubles, the time has always been in whole
> seconds.
Yep, I think it's ok.
Thanks,
Namhyung
Powered by blists - more mailing lists