Re: [PATCH] perf sched stats: Reject mismatched or incomplete snapshots

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Thu, Sep 17, 2026 at 12:11:25AM +0800, Tianyi Chen wrote:
> The before and after records are paired by list position. If a CPU or
> domain disappears, counters can be subtracted from a different record
> or left as absolute values. An extra record can also advance the cursor
> past the list.
> 
> Identify the second snapshot by its timestamp or CPU ordering, and
> require matching CPU/domain IDs and versions before subtracting. Check
> that every record has a counterpart before printing, and propagate
> errors from either input of diff.
> 
> Add a shell test with synthetic snapshots, including equal timestamps,
> CPU filtering, and missing or reordered CPU/domain records.
> 
> Fixes: 5a357ae6ad63 ("perf sched stats: Add support for report subcommand")

Swapnil, can you please ack/review this one?

- Arnaldo

> Assisted-by: LLM
> Signed-off-by: Tianyi Chen <hi@tychen.cc>
> ---
> The new shell test passes with GCC and Clang ASan/UBSan and fails on
> unpatched perf. Live and record/report/diff checks with temporary
> /proc/schedstat fixtures also pass.
> 
>  tools/perf/Documentation/perf-sched.txt       |   6 +-
>  tools/perf/builtin-sched.c                    | 189 ++++++++++++------
>  tools/perf/tests/shell/schedstat_snapshots.sh | 155 ++++++++++++++
>  3 files changed, 283 insertions(+), 67 deletions(-)
>  create mode 100755 tools/perf/tests/shell/schedstat_snapshots.sh
> 
> diff --git a/tools/perf/Documentation/perf-sched.txt b/tools/perf/Documentation/perf-sched.txt
> index 4da06215163a..fe0abef2bd13 100644
> --- a/tools/perf/Documentation/perf-sched.txt
> +++ b/tools/perf/Documentation/perf-sched.txt
> @@ -95,8 +95,10 @@ There are several variants of 'perf sched':
>     events, ``try_to_wakeup()`` call among others. This is useful in understanding the
>     scheduler behavior for the workload.
>  
> -   Note: The tool will not give correct results if there is topological reordering or
> -         online/offline of cpus in between capturing snapshots of `/proc/schedstat`.
> +   Note: Reports reject incomplete snapshots and mismatched CPU or domain IDs.
> +         Topology changes that retain the same IDs, including CPUs going offline
> +         and returning online between snapshots, cannot be detected and may still
> +         produce incorrect results.
>  
>      Example usage:
>          perf sched stats record -- sleep 1
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index dd39a4fb6c7a..9e7d170b30ad 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -4281,18 +4281,18 @@ struct schedstat_cpu {
>  };
>  
>  static struct list_head cpu_head = LIST_HEAD_INIT(cpu_head);
> -static struct schedstat_cpu *cpu_second_pass;
> -static struct schedstat_domain *domain_second_pass;
> +static struct list_head *cpu_second_pass;
> +static struct list_head *domain_second_pass;
> +static u64 schedstat_timestamp;
>  static bool after_workload_flag;
>  static bool verbose_field;
>  
>  static void free_schedstat(struct list_head *head);
>  
> -static void store_schedstat_cpu_diff(struct schedstat_cpu *after_workload)
> +static void store_schedstat_cpu_diff(struct perf_record_schedstat_cpu *before,
> +				    struct perf_record_schedstat_cpu *after)
>  {
> -	struct perf_record_schedstat_cpu *before = cpu_second_pass->cpu_data;
> -	struct perf_record_schedstat_cpu *after = after_workload->cpu_data;
> -	__u16 version = after_workload->cpu_data->version;
> +	__u16 version = after->version;
>  
>  #define CPU_FIELD(_type, _name, _desc, _format, _is_pct, _pct_of, _ver)	\
>  	(before->_ver._name = after->_ver._name - before->_ver._name)
> @@ -4308,11 +4308,10 @@ static void store_schedstat_cpu_diff(struct schedstat_cpu *after_workload)
>  #undef CPU_FIELD
>  }
>  
> -static void store_schedstat_domain_diff(struct schedstat_domain *after_workload)
> +static void store_schedstat_domain_diff(struct perf_record_schedstat_domain *before,
> +				       struct perf_record_schedstat_domain *after)
>  {
> -	struct perf_record_schedstat_domain *before = domain_second_pass->domain_data;
> -	struct perf_record_schedstat_domain *after = after_workload->domain_data;
> -	__u16 version = after_workload->domain_data->version;
> +	__u16 version = after->version;
>  
>  #define DOMAIN_FIELD(_type, _name, _desc, _format, _is_jiffies, _ver)	\
>  	(before->_ver._name = after->_ver._name - before->_ver._name)
> @@ -4814,12 +4813,35 @@ static int show_schedstat_data(struct list_head *head1, struct cpu_domain_map **
>   * other after completion of the workload. The above linked list stores the diff of the cpu and
>   * domain statistics.
>   */
> +static int schedstat_snapshot_error(void)
> +{
> +	pr_err("Incompatible or incomplete schedstat snapshots\n");
> +	return -EINVAL;
> +}
> +
> +static bool schedstat_domains_complete(void)
> +{
> +	struct schedstat_cpu *cpu;
> +
> +	if (!domain_second_pass)
> +		return true;
> +	cpu = list_entry(cpu_second_pass, struct schedstat_cpu, cpu_list);
> +	return domain_second_pass == &cpu->domain_head;
> +}
> +
> +static int schedstat_snapshots_complete(void)
> +{
> +	if (!after_workload_flag || !cpu_second_pass ||
> +	    cpu_second_pass->next != &cpu_head || !schedstat_domains_complete())
> +		return schedstat_snapshot_error();
> +	return 0;
> +}
> +
>  static int perf_sched__process_schedstat(const struct perf_tool *tool __maybe_unused,
>  					 struct perf_session *session __maybe_unused,
>  					 union perf_event *event)
>  {
>  	struct perf_cpu this_cpu;
> -	static __u32 initial_cpu;
>  
>  	switch (event->header.type) {
>  	case PERF_RECORD_SCHEDSTAT_CPU:
> @@ -4836,63 +4858,91 @@ static int perf_sched__process_schedstat(const struct perf_tool *tool __maybe_un
>  		return 0;
>  
>  	if (event->header.type == PERF_RECORD_SCHEDSTAT_CPU) {
> -		struct schedstat_cpu *temp = zalloc(sizeof(*temp));
> -
> -		if (!temp)
> -			return -ENOMEM;
> -
> -		temp->cpu_data = zalloc(sizeof(*temp->cpu_data));
> -		if (!temp->cpu_data)
> -			return -ENOMEM;
> +		struct perf_record_schedstat_cpu *data = &event->schedstat_cpu;
> +		struct schedstat_cpu *cpu;
>  
> -		memcpy(temp->cpu_data, &event->schedstat_cpu, sizeof(*temp->cpu_data));
> -
> -		if (!list_empty(&cpu_head) && temp->cpu_data->cpu == initial_cpu)
> -			after_workload_flag = true;
> -
> -		if (!after_workload_flag) {
> -			if (list_empty(&cpu_head))
> -				initial_cpu = temp->cpu_data->cpu;
> -
> -			list_add_tail(&temp->cpu_list, &cpu_head);
> -			INIT_LIST_HEAD(&temp->domain_head);
> -		} else {
> -			if (temp->cpu_data->cpu == initial_cpu) {
> -				cpu_second_pass = list_first_entry(&cpu_head, struct schedstat_cpu,
> -								   cpu_list);
> -				cpu_second_pass->cpu_data->timestamp =
> -					temp->cpu_data->timestamp - cpu_second_pass->cpu_data->timestamp;
> -			} else {
> -				cpu_second_pass = list_next_entry(cpu_second_pass, cpu_list);
> +		if (list_empty(&cpu_head)) {
> +			after_workload_flag = false;
> +			cpu_second_pass = &cpu_head;
> +			domain_second_pass = NULL;
> +			schedstat_timestamp = data->timestamp;
> +		} else if (!after_workload_flag) {
> +			cpu = list_last_entry(&cpu_head, struct schedstat_cpu, cpu_list);
> +			/* Snapshots share a timestamp and list CPUs in increasing order. */
> +			if (data->timestamp != schedstat_timestamp ||
> +			    data->cpu <= cpu->cpu_data->cpu) {
> +				after_workload_flag = true;
> +				schedstat_timestamp = data->timestamp;
>  			}
> -			domain_second_pass = list_first_entry(&cpu_second_pass->domain_head,
> -							      struct schedstat_domain, domain_list);
> -			store_schedstat_cpu_diff(temp);
> -			free(temp->cpu_data);
> -			free(temp);
>  		}
> -	} else if (event->header.type == PERF_RECORD_SCHEDSTAT_DOMAIN) {
> -		struct schedstat_cpu *cpu_tail;
> -		struct schedstat_domain *temp = zalloc(sizeof(*temp));
>  
> -		if (!temp)
> -			return -ENOMEM;
> +		if (after_workload_flag) {
> +			if (data->timestamp != schedstat_timestamp || !schedstat_domains_complete())
> +				return schedstat_snapshot_error();
> +			cpu_second_pass = cpu_second_pass->next;
> +			if (cpu_second_pass == &cpu_head)
> +				return schedstat_snapshot_error();
> +			cpu = list_entry(cpu_second_pass, struct schedstat_cpu, cpu_list);
> +			if (data->cpu != cpu->cpu_data->cpu ||
> +			    data->version != cpu->cpu_data->version ||
> +			    data->timestamp < cpu->cpu_data->timestamp)
> +				return schedstat_snapshot_error();
> +			cpu->cpu_data->timestamp = data->timestamp - cpu->cpu_data->timestamp;
> +			store_schedstat_cpu_diff(cpu->cpu_data, data);
> +			domain_second_pass = cpu->domain_head.next;
> +			return 0;
> +		}
>  
> -		temp->domain_data = zalloc(sizeof(*temp->domain_data));
> -		if (!temp->domain_data)
> +		cpu = zalloc(sizeof(*cpu));
> +		if (!cpu)
>  			return -ENOMEM;
> +		cpu->cpu_data = memdup(data, sizeof(*data));
> +		if (!cpu->cpu_data) {
> +			free(cpu);
> +			return -ENOMEM;
> +		}
> +		INIT_LIST_HEAD(&cpu->domain_head);
> +		list_add_tail(&cpu->cpu_list, &cpu_head);
> +	} else {
> +		struct perf_record_schedstat_domain *data = &event->schedstat_domain;
> +		struct schedstat_domain *domain;
> +		struct schedstat_cpu *cpu;
> +
> +		if (list_empty(&cpu_head) || data->timestamp != schedstat_timestamp)
> +			return schedstat_snapshot_error();
> +		if (after_workload_flag) {
> +			cpu = list_entry(cpu_second_pass, struct schedstat_cpu, cpu_list);
> +			if (domain_second_pass == &cpu->domain_head)
> +				return schedstat_snapshot_error();
> +			domain = list_entry(domain_second_pass, struct schedstat_domain,
> +					    domain_list);
> +			if (data->cpu != domain->domain_data->cpu ||
> +			    data->domain != domain->domain_data->domain ||
> +			    data->version != domain->domain_data->version)
> +				return schedstat_snapshot_error();
> +			store_schedstat_domain_diff(domain->domain_data, data);
> +			domain_second_pass = domain_second_pass->next;
> +			return 0;
> +		}
>  
> -		memcpy(temp->domain_data, &event->schedstat_domain, sizeof(*temp->domain_data));
> -
> -		if (!after_workload_flag) {
> -			cpu_tail = list_last_entry(&cpu_head, struct schedstat_cpu, cpu_list);
> -			list_add_tail(&temp->domain_list, &cpu_tail->domain_head);
> -		} else {
> -			store_schedstat_domain_diff(temp);
> -			domain_second_pass = list_next_entry(domain_second_pass, domain_list);
> -			free(temp->domain_data);
> -			free(temp);
> +		cpu = list_last_entry(&cpu_head, struct schedstat_cpu, cpu_list);
> +		if (data->cpu != cpu->cpu_data->cpu || data->version != cpu->cpu_data->version)
> +			return schedstat_snapshot_error();
> +		if (!list_empty(&cpu->domain_head)) {
> +			domain = list_last_entry(&cpu->domain_head, struct schedstat_domain,
> +						 domain_list);
> +			if (data->domain <= domain->domain_data->domain)
> +				return schedstat_snapshot_error();
>  		}
> +		domain = zalloc(sizeof(*domain));
> +		if (!domain)
> +			return -ENOMEM;
> +		domain->domain_data = memdup(data, sizeof(*data));
> +		if (!domain->domain_data) {
> +			free(domain);
> +			return -ENOMEM;
> +		}
> +		list_add_tail(&domain->domain_list, &cpu->domain_head);
>  	}
>  
>  	return 0;
> @@ -4947,6 +4997,8 @@ static int perf_sched__schedstat_report(struct perf_sched *sched)
>  	user_requested_cpus = evlist__core(session->evlist)->user_requested_cpus;
>  
>  	err = perf_session__process_events(session);
> +	if (!err)
> +		err = schedstat_snapshots_complete();
>  
>  	if (!err) {
>  		setup_pager();
> @@ -4976,7 +5028,7 @@ static int perf_sched__schedstat_diff(struct perf_sched *sched,
>  	struct list_head cpu_head_ses0, cpu_head_ses1;
>  	struct perf_session *session[2];
>  	struct perf_data data[2] = {0};
> -	int ret = 0, err = 0;
> +	int ret = 0;
>  	static const char *defaults[] = {
>  		"perf.data.old",
>  		"perf.data",
> @@ -5009,8 +5061,10 @@ static int perf_sched__schedstat_diff(struct perf_sched *sched,
>  		goto out_delete_ses0;
>  	}
>  
> -	err = perf_session__process_events(session[0]);
> -	if (err) {
> +	ret = perf_session__process_events(session[0]);
> +	if (!ret)
> +		ret = schedstat_snapshots_complete();
> +	if (ret) {
>  		free_schedstat(&cpu_head);
>  		goto out_delete_ses0;
>  	}
> @@ -5028,8 +5082,10 @@ static int perf_sched__schedstat_diff(struct perf_sched *sched,
>  		goto out_delete_ses1;
>  	}
>  
> -	err = perf_session__process_events(session[1]);
> -	if (err) {
> +	ret = perf_session__process_events(session[1]);
> +	if (!ret)
> +		ret = schedstat_snapshots_complete();
> +	if (ret) {
>  		free_schedstat(&cpu_head);
>  		goto out_delete_ses1;
>  	}
> @@ -5152,6 +5208,9 @@ static int perf_sched__schedstat_live(struct perf_sched *sched,
>  					       user_requested_cpus);
>  	if (err)
>  		goto out;
> +	err = schedstat_snapshots_complete();
> +	if (err)
> +		goto out;
>  
>  	setup_pager();
>  
> diff --git a/tools/perf/tests/shell/schedstat_snapshots.sh b/tools/perf/tests/shell/schedstat_snapshots.sh
> new file mode 100755
> index 000000000000..a4bdf2673d43
> --- /dev/null
> +++ b/tools/perf/tests/shell/schedstat_snapshots.sh
> @@ -0,0 +1,155 @@
> +#!/bin/sh
> +# Validate CPU and domain pairing in perf sched stats snapshots
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +# shellcheck source=lib/setup_python.sh
> +. "$(dirname "$0")/lib/setup_python.sh"
> +
> +if ! perf version --build-options | grep -q 'libtraceevent:.*on'; then
> +	echo "[Skip] perf sched requires libtraceevent"
> +	exit 2
> +fi
> +
> +$PYTHON - <<'PY'
> +import os
> +import re
> +import struct
> +import subprocess
> +import sys
> +import tempfile
> +
> +# Native-endian perf.data with only NRCPUS and CPU_DOMAIN_INFO features.
> +endian = '<' if sys.byteorder == 'little' else '>'
> +
> +
> +def pack(fmt, *values):
> +    return struct.pack(endian + fmt, *values)
> +
> +
> +def string(value):
> +    data = value.encode() + b'\0'
> +    return pack('I', len(data)) + data
> +
> +
> +def cpu(cpu_id, timestamp, value, version):
> +    return pack('IHHQIHH6I3Q', 85, 0, 72, timestamp, cpu_id, version, 0,
> +                *([value] * 9))
> +
> +
> +def domain(cpu_id, domain_id, timestamp, value, version):
> +    # All supported versions use the largest union member's record size.
> +    return pack('IHHQIHH45I4x', 86, 0, 208, timestamp, cpu_id, version,
> +                domain_id, *([value] * 45))
> +
> +
> +def snapshot(timestamp, value, version=17, cpus=(0, 1, 2), domains=(0, 1)):
> +    records = []
> +    for cpu_id in cpus:
> +        records.append(cpu(cpu_id, timestamp, value + cpu_id * 100, version))
> +        for domain_id in domains:
> +            records.append(domain(cpu_id, domain_id, timestamp,
> +                                  value + cpu_id * 100, version))
> +    return records
> +
> +
> +def write_file(path, records, version=17):
> +    metadata = pack('II', version, 2)
> +    for cpu_id in range(3):
> +        metadata += pack('II', cpu_id, 2)
> +        for domain_id in range(2):
> +            metadata += pack('I', domain_id)
> +            if version >= 17:
> +                metadata += string('SMT' if domain_id == 0 else 'MC')
> +            metadata += string('7') + string('0-2')
> +    features = [pack('II', 3, 3), metadata]
> +    data = b''.join(records)
> +    offset = 104 + len(data) + 16 * len(features)
> +    sections = b''
> +    for feature in features:
> +        sections += pack('QQ', offset, len(feature))
> +        offset += len(feature)
> +    header = pack('13Q', 0x32454c4946524550, 104, 144, 104, 0,
> +                  104, len(data), 0, 0, (1 << 7) | (1 << 32), 0, 0, 0)
> +    with open(path, 'wb') as output:
> +        output.write(header + data + sections + b''.join(features))
> +
> +
> +def run(args, valid, domains=True):
> +    result = subprocess.run(['perf', 'sched', 'stats'] + args,
> +                            stdout=subprocess.PIPE, stderr=subprocess.PIPE,
> +                            text=True, timeout=10)
> +    if valid:
> +        assert result.returncode == 0, result.stderr
> +        assert re.search(r'^yld_count\s+:\s+10\b', result.stdout, re.M), result.stdout
> +        if domains:
> +            assert re.search(r'^busy_lb_count\s+:\s+10\b', result.stdout, re.M), result.stdout
> +        else:
> +            assert 'busy_lb_count' not in result.stdout, result.stdout
> +    else:
> +        assert result.returncode > 0, (args, result.returncode, result.stdout)
> +        assert 'Incompatible or incomplete schedstat snapshots' in result.stderr
> +        assert not result.stdout, result.stdout
> +    assert 'Sanitizer' not in result.stderr, result.stderr
> +
> +
> +with tempfile.TemporaryDirectory(prefix='perf-schedstat-') as directory:
> +    good = os.path.join(directory, 'good.data')
> +    test = os.path.join(directory, 'test.data')
> +    before = snapshot(100, 1000)
> +    after = snapshot(200, 1010)
> +    write_file(good, before + after)
> +
> +    for version in (15, 16, 17):
> +        for timestamp in (100, 200):
> +            write_file(test, snapshot(100, 1000, version) +
> +                       snapshot(timestamp, 1010, version), version)
> +            run(['report', '-C', '0,1,2', '-i', test], True)
> +            run(['diff', test, test], True)
> +    print('Matching snapshots, including equal timestamps: [Success]')
> +
> +    write_file(test, snapshot(100, 1000, domains=()) +
> +               snapshot(200, 1010, domains=()))
> +    run(['report', '-C', '0,1,2', '-i', test], True, domains=False)
> +    run(['diff', test, test], True, domains=False)
> +    print('CPUs without domains: [Success]')
> +
> +    write_file(test, before + snapshot(200, 1010, cpus=(0, 1)))
> +    run(['report', '-C', '0,1', '-i', test], True)
> +    run(['report', '-C', '1', '-i', good], True)
> +    write_file(test, snapshot(100, 1000, cpus=(0, 1)) +
> +               snapshot(200, 1010, cpus=(0, 1)))
> +    run(['diff', good, test], True)
> +    print('CPU filtering and different CPU sets across files: [Success]')
> +
> +    write_file(test, snapshot(100, 0xfffffffa, cpus=(0,)) +
> +               snapshot(200, 4, cpus=(0,)))
> +    run(['report', '-C', '0', '-i', test], True)
> +    print('Wrapping 32-bit counters: [Success]')
> +
> +    cases = {
> +        'missing first CPU': before + snapshot(200, 1010, cpus=(1, 2)),
> +        'missing middle CPU': before + snapshot(200, 1010, cpus=(0, 2)),
> +        'missing last CPU': before + snapshot(200, 1010, cpus=(0, 1)),
> +        'added CPU': snapshot(100, 1000, cpus=(0, 1)) + after,
> +        'reordered CPUs': before + snapshot(200, 1010, cpus=(0, 2, 1)),
> +        'equal timestamp, missing first CPU': before + snapshot(100, 1010, cpus=(1, 2)),
> +        'missing first domain': before + snapshot(200, 1010, domains=(1,)),
> +        'missing last domain': before + after[:-1],
> +        'added domain': snapshot(100, 1000, domains=(0,)) + after,
> +        'reordered domains': before + snapshot(200, 1010, domains=(1, 0)),
> +        'domain without CPU': before[1:] + after,
> +        'wrong domain CPU': before + [after[0], domain(1, 0, 200, 1010, 17)] + after[2:],
> +        'changed version': before + snapshot(200, 1010, version=16),
> +        'backwards timestamp': before + snapshot(50, 1010),
> +        'third snapshot': before + after + snapshot(300, 1020),
> +        'missing second snapshot': before,
> +    }
> +    for name, records in cases.items():
> +        write_file(test, records)
> +        run(['report', '-C', '0,1,2', '-i', test], False)
> +        run(['diff', test, good], False)
> +        run(['diff', good, test], False)
> +        print(name + ': [Success]')
> +PY
> 
> base-commit: 91b0782fc9e9d2f0a40b5256146e014802fdbb36
> -- 
> 2.55.0