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

[Swapnil Sapkal <swapnil.sapkal@amd.com>]

Hi Arnaldo, Tianyi,

Sorry for the delay in response. I was on vacation last week.

On 25-09-2026 18:37, Arnaldo Carvalho de Melo wrote:
> 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
> 

I have reviewed and tested the changes. It looks good to me. Please feel 
free to add

Reviewed-and-tested-by: Swapnil Sapkal <swapnil.sapkal@amd.com>

--
Thanks and Regards,
Swapnil

>> 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