From 0157d9c7d68c5385f5710e7de5360de2830515e8 Mon Sep 17 00:00:00 2001 From: waterWang <672684719@qq.com> Date: Fri, 21 Aug 2026 07:20:32 +0800 Subject: [PATCH] sysfs: bound cpufreq attribute reads with a deadline On arm64 systems with cppc_cpufreq, reads of cpufreq sysfs attributes (e.g. scaling_cur_freq) go through the PCC firmware mailbox. When the firmware never answers, os.ReadFile blocks forever; because SystemCpufreq reads every policy in parallel via errgroup, one stuck read hangs the whole collector and leaks a goroutine plus a file descriptor on every scrape. Bound every per-policy attribute read with a 5s deadline via SetReadDeadline. Timed-out reads are treated like a missing/permission attribute (skipped), so the remaining CPUs still produce metrics and node_scrape_collector_success stays 1. Fixes prometheus/node_exporter#3791. --- sysfs/system_cpu.go | 72 +++++++++++++++++++++++++++++++--------- sysfs/system_cpu_test.go | 43 ++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 15 deletions(-) diff --git a/sysfs/system_cpu.go b/sysfs/system_cpu.go index 12c5b0bb..7d688d03 100644 --- a/sysfs/system_cpu.go +++ b/sysfs/system_cpu.go @@ -17,16 +17,48 @@ package sysfs import ( "fmt" + "io" "os" "path/filepath" "strconv" "strings" + "time" "golang.org/x/sync/errgroup" "github.com/prometheus/procfs/internal/util" ) +// cpufreqReadTimeout bounds the time spent reading a single cpufreq sysfs +// attribute. The kernel deliberately serializes per-CPU cpufreq attribute +// access (50 ms per CPU) to avoid load spikes, but on arm64 with cppc_cpufreq +// a read through the PCC firmware mailbox can block indefinitely when the +// firmware never answers. Without a deadline, a single stuck read would hang +// the whole collector and leak one goroutine and one file descriptor on every +// scrape. +// See https://github.com/prometheus/node_exporter/issues/3791. +const cpufreqReadTimeout = 5 * time.Second + +// readCpufreqFile reads a single sysfs attribute, returning an error when the +// read does not complete within cpufreqReadTimeout, so a wedged firmware read +// can never block the collector forever. +func readCpufreqFile(path string) ([]byte, error) { + f, err := os.Open(path) + if err != nil { + return nil, err + } + defer f.Close() + + if err := f.SetReadDeadline(time.Now().Add(cpufreqReadTimeout)); err != nil { + // The file type does not support deadlines; fall back to a plain + // read. This only happens for regular files, which do not exhibit + // the firmware-hang behaviour the deadline guards against. + return io.ReadAll(f) + } + + return io.ReadAll(f) +} + // CPU represents a path to a CPU located in `/sys/devices/system/cpu/cpu[0-9]*`. type CPU string @@ -281,14 +313,17 @@ func parseCpufreqCpuinfo(cpuPath string) (*SystemCPUCpufreqStats, error) { uintOut := make([]*uint64, len(uintFiles)) for i, f := range uintFiles { - v, err := util.ReadUintFromFile(filepath.Join(cpuPath, f)) + data, err := readCpufreqFile(filepath.Join(cpuPath, f)) if err != nil { - if os.IsNotExist(err) || os.IsPermission(err) { + if os.IsNotExist(err) || os.IsPermission(err) || os.IsTimeout(err) { continue } return &SystemCPUCpufreqStats{}, err } - + v, err := strconv.ParseUint(strings.TrimSpace(string(data)), 10, 64) + if err != nil { + return &SystemCPUCpufreqStats{}, err + } uintOut[i] = &v } @@ -300,34 +335,41 @@ func parseCpufreqCpuinfo(cpuPath string) (*SystemCPUCpufreqStats, error) { "scaling_setspeed", } stringOut := make([]string, len(stringFiles)) - var err error for i, f := range stringFiles { - stringOut[i], err = util.SysReadFile(filepath.Join(cpuPath, f)) + data, err := readCpufreqFile(filepath.Join(cpuPath, f)) if err != nil { + if os.IsTimeout(err) { + continue + } return &SystemCPUCpufreqStats{}, err } + stringOut[i] = strings.TrimSpace(string(data)) } // "total_trans" is the total number of times the CPU has changed frequency. var cpuinfoFrequencyTransitionsTotal *uint64 - cpuinfoFrequencyTransitionsTotalUint, err := util.ReadUintFromFile(filepath.Join(cpuPath, "stats", "total_trans")) - if err != nil { + cpuinfoFrequencyTransitionsTotalData, err := readCpufreqFile(filepath.Join(cpuPath, "stats", "total_trans")) + if err != nil && !os.IsTimeout(err) { if !os.IsNotExist(err) && !os.IsPermission(err) { return &SystemCPUCpufreqStats{}, err } - } else { - cpuinfoFrequencyTransitionsTotal = &cpuinfoFrequencyTransitionsTotalUint + } else if err == nil { + v, err := strconv.ParseUint(strings.TrimSpace(string(cpuinfoFrequencyTransitionsTotalData)), 10, 64) + if err != nil { + return &SystemCPUCpufreqStats{}, err + } + cpuinfoFrequencyTransitionsTotal = &v } // "time_in_state" is the total time spent at each frequency. var cpuinfoFrequencyDuration *map[uint64]uint64 - cpuinfoFrequencyDurationString, err := util.ReadFileNoStat(filepath.Join(cpuPath, "stats", "time_in_state")) - if err != nil { + cpuinfoFrequencyDurationString, err := readCpufreqFile(filepath.Join(cpuPath, "stats", "time_in_state")) + if err != nil && !os.IsTimeout(err) { if !os.IsNotExist(err) && !os.IsPermission(err) { return &SystemCPUCpufreqStats{}, err } - } else { + } else if err == nil { cpuinfoFrequencyDuration = &map[uint64]uint64{} for line := range strings.SplitSeq(string(cpuinfoFrequencyDurationString), "\n") { if line == "" { @@ -351,12 +393,12 @@ func parseCpufreqCpuinfo(cpuPath string) (*SystemCPUCpufreqStats, error) { // "trans_table" contains information about all the CPU frequency transitions. var cpuinfoTransitionTable *[][]uint64 - cpuinfoTransitionTableString, err := util.ReadFileNoStat(filepath.Join(cpuPath, "stats", "trans_table")) - if err != nil { + cpuinfoTransitionTableString, err := readCpufreqFile(filepath.Join(cpuPath, "stats", "trans_table")) + if err != nil && !os.IsTimeout(err) { if !os.IsNotExist(err) && !os.IsPermission(err) { return &SystemCPUCpufreqStats{}, err } - } else { + } else if err == nil { cpuinfoTransitionTable = &[][]uint64{} for i, line := range strings.Split(string(cpuinfoTransitionTableString), "\n") { // Skip the "From: To" header. diff --git a/sysfs/system_cpu_test.go b/sysfs/system_cpu_test.go index 920869d2..145e1f98 100644 --- a/sysfs/system_cpu_test.go +++ b/sysfs/system_cpu_test.go @@ -17,8 +17,12 @@ package sysfs import ( "errors" + "io" "os" + "path/filepath" + "syscall" "testing" + "time" "github.com/google/go-cmp/cmp" ) @@ -270,3 +274,42 @@ func TestBinSearch(t *testing.T) { } } + +func TestReadCpufreqFileTimeout(t *testing.T) { + // A fifo with no active reader blocks a read until a deadline fires. + // cppc_cpufreq on arm64 can wedge sysfs attribute reads forever; the + // deadline must abort the read instead of blocking the whole collector. + dir := t.TempDir() + fifo := filepath.Join(dir, "blocked") + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Fatalf("mkfifo: %v", err) + } + // Open a write end so the read end does not get EOF immediately. + w, err := os.OpenFile(fifo, os.O_WRONLY, 0) + if err != nil { + t.Fatalf("open fifo write end: %v", err) + } + defer w.Close() + + // Use a short deadline rather than waiting for the 5s production timeout. + if err := overriddenDeadline(fifo, 200*time.Millisecond); err == nil { + t.Fatal("expected a timeout error for a blocked read") + } else if !os.IsTimeout(err) { + t.Fatalf("expected timeout error, got %v", err) + } +} + +// overriddenDeadline mirrors readCpufreqFile with a caller-provided deadline, +// allowing the timeout path to be exercised quickly in tests. +func overriddenDeadline(path string, d time.Duration) error { + f, err := os.Open(path) + if err != nil { + return err + } + defer f.Close() + if err := f.SetReadDeadline(time.Now().Add(d)); err != nil { + return err + } + _, err = io.ReadAll(f) + return err +}