Hi Andi,
> On 19 Sep 2026, at 1:46 pm, Andi Kleen <[email protected]> wrote: > > External email: Use caution opening links or attachments > > > On Thu, Sep 17, 2026 at 10:39:13AM +1000, Kugan Vivekanandarajah wrote: >> + size_t processed_samples = 0; >> + >> + /* MMAPs can appear after samples in the ring buffer; bind mappings >> first. */ >> + if (!for_each_mapped_perf_data_record ( >> + [&] (const perf_event_header &record_header, >> + const uint8_t *payload_ptr, >> + size_t payload_size, >> + size_t) >> + { >> + switch (record_header.type) >> + { >> + case perf_record_type::MMAP: >> + parse_mmap_record (payload_ptr, payload_size, >> + record_header.misc); >> + break; >> + case perf_record_type::MMAP2: >> + parse_mmap2_record (payload_ptr, payload_size, >> + record_header.misc); >> + break; > > So the problem with the two step walk is that you don't handle the case > when a library gets unloaded and something else loaded in the same > place. > > I realize this might be hard to handle with your other data structures > that assume an unique PC mapping, but it would be good to at least detect > and warn about it. > Good catch. We can use MMAP timestamps to give each mapping a valid time window. PCs are matched to the mapping active at the time of the sample. > >> + case perf_record_type::COMM: >> + note_process_lifetime_record ( >> + record_header, payload_ptr, payload_size); >> + break; >> + default: >> + break; > > It would probably be useful to handle FINISHED_ROUND at least, > I think those can happen with aio writes etc. perf emits one after every sweep of the ring buffers too. With the timestamp based mapping validity windows above, we compare absolute timestamps instead, so the round boundaries would not add anything.So I would document why we skip it rather than handle it. Does that seem reasonable? > >> +bool >> +perf_branch_stack_reader::parse_sample_record (const uint8_t *record_data, >> + size_t record_size) >> +{ >> + if (use_fixed_layout_branch_stack_parse_) >> + return parse_sample_record_fixed_branch_stack_layout ( >> + record_data, record_size); >> + >> + const uint8_t *ptr = record_data; >> + const uint8_t *const end = record_data + record_size; >> + >> + uint64_t ip = 0; >> + bool have_ip = false; >> + uint32_t pid = 0; >> + uint64_t sample_period = 0; >> + uint64_t sample_time = 0; >> + >> + if (!perf_event_sample_util::walk_sample_before_branch ( >> + ptr, end, sample_layout.sample_type, sample_layout.read_format, &ip, >> + &have_ip, &pid, nullptr, &sample_period, &sample_time)) >> + { >> + fprintf (stderr, >> + _("PERF: WARNING: Truncated or invalid sample (prefix)\n")); >> + return false; >> + } > > Probably need some kind of rate limit on the warning here, also for the > others. Otherwise it could be a very long flood of messages for a bad > file. Agreed. I would print the first warning, count the rest, and report the total once at the end. > >> + if (sample_layout.fixed_sample_period >> + && sample_period != sample_layout.sample_period) >> + { >> + fprintf (stderr, >> + _("PERF: WARNING: sample period does not match the fixed " >> + "event period\n")); >> + return false; >> + } > > > Perf can actually reduce the period any time when it is throttling > because of PMI overload. Better just handle it silently. Ok. > >> + ptr, end, sample_layout.sample_type, sample_layout.sample_regs_user, >> + sample_layout.sample_regs_intr) >> + || ptr != end) >> + { >> + fprintf (stderr, >> + _("PERF: WARNING: Truncated or invalid sample suffix\n")); >> + return false; >> + } >> + >> + const std::vector<simple_mmap> *const pid_mmaps = target_mmaps_for_pid >> (pid); >> + if (pid_mmaps == nullptr) >> + return false; >> + uint64_t dwarf_ip = 0; >> + const bool ip_is_target >> + = have_ip && ip != 0 >> + && try_translate_runtime_with_mmaps (*pid_mmaps, ip, &dwarf_ip); >> + bool recorded_sample = record_sample_ip ( >> + dwarf_ip, ip_is_target, sample_time, accumulate_sample_ips_, >> + address_frequencies); >> + if (use_branch_stack_for_correlation_ >> + && (sample_layout.sample_type & perf_sample_type::BRANCH_STACK) != 0 >> + && nr > 0 >> + && accumulate_branch_stack_sample ( >> + entries, >> + nr, >> + *this, >> + sample_time, > > > I forgot this earlier, but it would be good if you supported not having > a time stamp to shrink the files. Reading a perf.data without timestamps works. The layout comes from the event attr, so the field is simply absent. It gives the first execution time of each function, which the profile records and GCC turns into node->tp_first_run for function reordering > >> + const uint8_t *record_data, size_t record_size) >> +{ >> + if (record_size < BRANCH_STACK_FIXED_PREFIX_BYTES + sizeof (uint64_t)) >> + return false; > > What's the extra 8 bytes? It is the branch stack entry count. PERF_SAMPLE_BRANCH_STACK starts with a u64 nr before the entries, so that is the shortest record with anything to parse. I would name it rather than add it inline as BRANCH_STACK_FIXED_PREFIX_AND_COUNT_BYTES I am revising the patch series and will post the revised (includining addressing other review comments) Thanks, Kugan > > -Andi
