Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] The `mm_vmscan_lru_shrink_inactive` tracepoint prints `nr_ref_keep` twice and omits the newly added `nr_ref_activate` field. - [Medium] Missing TRACE_DEFINE_ENUM exports for PGSTEAL_* enum values used in trace_reclaim_reason_ops. - [Medium] The patch breaks the in-tree `trace-vmscan-postprocess.pl` script by altering tracepoint arguments without updating the script. --
commit 415a9b2c51b867b9381070883253c78fbde68aee Author: qiwu.chen <[email protected]> mm: vmscan: rework lru_shrink and write_folio tracepoints This commit reworks the lru_shrink and write_folio tracepoints for better correlation and analysis. It traces each folio LRU type and reclaim reason, adds activation statistics, and removes some unused reclaim flags. > diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h > index 8a872990b4bee..db6c6d2d777af 100644 > --- a/include/trace/events/vmscan.h > +++ b/include/trace/events/vmscan.h [ ... ] > -#define trace_reclaim_flags(file) ( \ > - (file ? RECLAIM_WB_FILE : RECLAIM_WB_ANON) | \ > - (RECLAIM_WB_ASYNC) \ > - ) > +#define trace_reclaim_reason_ops \ > + {PGSTEAL_KSWAPD, "KSWAPD"}, \ > + {PGSTEAL_DIRECT, "DIRECT"}, \ > + {PGSTEAL_KHUGEPAGED, "KHUGEPAGED"}, \ > + {PGSTEAL_PROACTIVE, "PROACTIVE"} [Severity: Medium] Since this macro maps enums to strings for use with __print_symbolic(), do these enum values need corresponding TRACE_DEFINE_ENUM() exports? Without them, userspace tools like perf or trace-cmd will not be able to evaluate the enum values from the trace format file and will fail to parse the reason field. [ ... ] > @@ -495,29 +480,32 @@ TRACE_EVENT(mm_vmscan_lru_shrink_inactive, > __entry->nr_activate0 = stat->nr_activate[0]; > __entry->nr_activate1 = stat->nr_activate[1]; > __entry->nr_ref_keep = stat->nr_ref_keep; > + __entry->nr_ref_activate = stat->nr_ref_activate; > __entry->nr_unmap_fail = stat->nr_unmap_fail; > __entry->priority = priority; > - __entry->reclaim_flags = trace_reclaim_flags(file); > + __entry->lru = lru; > + __entry->reason = reason; > ), > > - TP_printk("nid=%d nr_scanned=%ld nr_reclaimed=%ld nr_dirty=%ld > nr_writeback=%ld nr_congested=%ld nr_immediate=%ld nr_activate_anon=%d > nr_activate_file=%d nr_ref_keep=%ld nr_unmap_fail=%ld priority=%d flags=%s", > + TP_printk("nid=%d nr_scanned=%ld nr_reclaimed=%ld nr_dirty=%ld > nr_writeback=%ld nr_congested=%ld nr_immediate=%ld nr_activate_anon=%d > nr_activate_file=%d nr_ref_keep=%ld nr_ref_activate=%ld nr_unmap_fail=%ld > priority=%d lru=%s reason=%s", > __entry->nid, > __entry->nr_scanned, __entry->nr_reclaimed, > __entry->nr_dirty, __entry->nr_writeback, > __entry->nr_congested, __entry->nr_immediate, > __entry->nr_activate0, __entry->nr_activate1, > - __entry->nr_ref_keep, __entry->nr_unmap_fail, > - __entry->priority, > - show_reclaim_flags(__entry->reclaim_flags)) > + __entry->nr_ref_keep, __entry->nr_ref_keep, [Severity: Medium] Is the second __entry->nr_ref_keep here a typo? It looks like it should be __entry->nr_ref_activate to match the format string. The duplicated nr_ref_keep argument causes the correct nr_ref_activate field to be ignored in the trace output. [Severity: Medium] Does the Documentation/trace/postprocess/trace-vmscan-postprocess.pl script also need to be updated to handle this new format? The script contains a hardcoded regular expression for this tracepoint: $regex_lru_shrink_inactive = generate_traceevent_regex( "vmscan/mm_vmscan_lru_shrink_inactive", $regex_lru_shrink_inactive_default, "nid", "nr_scanned", "nr_reclaimed", "nr_dirty", "nr_writeback", "nr_congested", "nr_immediate", "nr_activate_anon", "nr_activate_file", "nr_ref_keep", "nr_unmap_fail", "priority", "flags"); Since flags was renamed to lru, and reason and nr_ref_activate were added, the parsing logic in the script will fail on traces using this new format. > + __entry->nr_unmap_fail, __entry->priority, > + __print_symbolic(__entry->lru, LRU_NAMES), > + __print_symbolic(__entry->reason, trace_reclaim_reason_ops)) > ); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
