Merge pull request #2128 from shirou/feat/follow-up-2125

[cpu][windows]: harden the cpu-total computation added in #2125

Author: shirou

cpu/cpu_psutil_test.go

@@ -82,8 +82,10 @@ func TestTimes_Against_Psutil(t *testing.T) {
 	require.NoError(t, err)
 
 	// user/system/idle exist on all supported platforms and both sides
-	// derive them identically (linux: /proc/stat; windows: GetSystemTimes
-	// with system = kernel - idle; darwin: mach host statistics).
+	// derive them from the same counters (linux: /proc/stat; darwin: mach host
+	// statistics; windows: system = kernel - idle, which psutil reads via
+	// GetSystemTimes while gopsutil sums the per-CPU counters — equivalent as
+	// long as the host has a single processor group, see #2125).
 	pt.AssertBracketedDelta(t, "User", before.User, got.User, after.User, cpuTimesSlack)
 	pt.AssertBracketedDelta(t, "System", before.System, got.System, after.System, cpuTimesSlack)
 	pt.AssertBracketedDelta(t, "Idle", before.Idle, got.Idle, after.Idle, cpuTimesSlack)

cpu/cpu_windows.go

@@ -106,8 +106,17 @@ func Times(percpu bool) ([]TimesStat, error) {
 func TimesWithContext(_ context.Context, percpu bool) ([]TimesStat, error) {
 	// Get the CPU performance counters per processor via Windows API
 	stats, err := perfInfo()
+	if err == nil && len(stats) == 0 {
+		err = errors.New("no processor performance information returned")
+	}
 	if err != nil {
-		return nil, err
+		if percpu {
+			return nil, err
+		}
+		// perfInfo relies on the undocumented NtQuerySystemInformation, which
+		// may be unavailable in restricted environments. Fall back to the
+		// public GetSystemTimes for the total rather than failing outright.
+		return systemTimes()
 	}
 
 	if percpu {
@@ -130,23 +139,64 @@ func TimesWithContext(_ context.Context, percpu bool) ([]TimesStat, error) {
 	// machines with more than 64 logical processors when the current thread is
 	// switched to another processor group as then the counters are not
 	// monotonic anymore.
-	var total win32_SystemProcessorPerformanceInformation
+	//
+	// Do all arithmetic on the integer tick counts and convert to float64 only
+	// once. Converting each counter first loses precision on large counters and
+	// can make the returned values non-monotonic. See issue #2110.
+	var idle, kernel, user, interrupt int64
 	for _, v := range stats {
-		total.IdleTime += v.IdleTime
-		total.KernelTime += v.KernelTime
-		total.UserTime += v.UserTime
-		total.DpcTime += v.DpcTime
-		total.InterruptTime += v.InterruptTime
-		total.InterruptCount += v.InterruptCount
+		idle += v.IdleTime
+		kernel += v.KernelTime
+		user += v.UserTime
+		interrupt += v.InterruptTime
 	}
 
 	return []TimesStat{
 		{
 			CPU:    "cpu-total",
-			User:   float64(total.UserTime) / ClocksPerSec,
-			System: float64(total.KernelTime-total.IdleTime) / ClocksPerSec,
-			Idle:   float64(total.IdleTime) / ClocksPerSec,
-			Irq:    float64(total.InterruptTime) / ClocksPerSec,
+			User:   float64(user) / ClocksPerSec,
+			System: float64(kernel-idle) / ClocksPerSec, // kernel time includes idle time
+			Idle:   float64(idle) / ClocksPerSec,
+			Irq:    float64(interrupt) / ClocksPerSec,
+		},
+	}, nil
+}
+
+// systemTimes returns the combined CPU times reported by GetSystemTimes.
+//
+// This is only a fallback for TimesWithContext(ctx, false): GetSystemTimes is a
+// documented public API but returns the counters of the calling thread's
+// processor group only, so on hosts with more than 64 logical processors the
+// values are not monotonic once the thread is migrated to another group. It
+// also cannot report Irq, which is left at zero here.
+//
+// Whether perfInfo works is a property of the environment, so in practice a
+// process stays on one path for its whole lifetime. Should perfInfo fail only
+// intermittently on a multi-group host, the two paths cover a different number
+// of processors, and Percent may report one bogus sample before recovering.
+func systemTimes() ([]TimesStat, error) {
+	var lpIdleTime, lpKernelTime, lpUserTime common.FILETIME
+	// GetSystemTimes returns 0 for error, in which case we check err,
+	// see https://pkg.go.dev/golang.org/x/sys/windows#LazyProc.Call
+	r, _, err := common.ProcGetSystemTimes.Call(
+		uintptr(unsafe.Pointer(&lpIdleTime)),
+		uintptr(unsafe.Pointer(&lpKernelTime)),
+		uintptr(unsafe.Pointer(&lpUserTime)))
+	if r == 0 {
+		return nil, err
+	}
+
+	// See the note on integer arithmetic in TimesWithContext and issue #2110.
+	idle := uint64(lpIdleTime.DwHighDateTime)<<32 | uint64(lpIdleTime.DwLowDateTime)
+	user := uint64(lpUserTime.DwHighDateTime)<<32 | uint64(lpUserTime.DwLowDateTime)
+	kernel := uint64(lpKernelTime.DwHighDateTime)<<32 | uint64(lpKernelTime.DwLowDateTime)
+
+	return []TimesStat{
+		{
+			CPU:    "cpu-total",
+			User:   float64(user) / ClocksPerSec,
+			System: float64(kernel-idle) / ClocksPerSec, // kernel time includes idle time
+			Idle:   float64(idle) / ClocksPerSec,
 		},
 	}, nil
 }
@@ -261,9 +311,16 @@ func perfInfo() ([]win32_SystemProcessorPerformanceInformation, error) {
 	// (up to 64 logical CPUs per group). The non-Ex NtQuerySystemInformation only returns
 	// data for the calling thread's group, so whenever the Ex variant is available we
 	// iterate every active group and concatenate the results. See issue #887.
-	if common.ProcNtQuerySystemInformationEx.Find() == nil {
+	//
+	// Every proc is resolved with Find before it is used: LazyProc.Call panics when it
+	// cannot resolve, and a panic would take the caller's process down instead of
+	// letting TimesWithContext fall back to GetSystemTimes.
+	if common.ProcNtQuerySystemInformationEx.Find() == nil && procGetActiveProcessorGroupCount.Find() == nil {
 		return perfInfoAllGroups()
 	}
+	if err := common.ProcNtQuerySystemInformation.Find(); err != nil {
+		return nil, err
+	}
 	return perfInfoSingleGroup()
 }
 

cpu/cpu_windows_test.go

@@ -6,10 +6,18 @@ package cpu
 import (
 	"context"
 	"testing"
+	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+// timesTotalSlack is the per-logical-CPU margin added on top of the measured
+// wall clock time when comparing cpu-total against the sum of the per-CPU stats.
+// It only has to absorb the counter granularity, since the time the two Times
+// calls are actually apart is measured rather than assumed.
+const timesTotalSlack = 0.1 // seconds per logical CPU
+
 // TestPerfInfoMatchesLogicalCount ensures perfInfo() returns one entry per logical
 // CPU on the host. This guards against regressions like issue #887 where only the
 // calling thread's processor group was reported on hosts with more than 64 CPUs.
@@ -22,3 +30,73 @@ func TestPerfInfoMatchesLogicalCount(t *testing.T) {
 
 	require.Len(t, info, n, "perfInfo must return one entry per logical CPU across all processor groups")
 }
+
+// TestTimesTotalMatchesPerCPUSum ensures the cpu-total entry is the field-wise sum
+// of the per-CPU entries. Both are derived from the same perfInfo() counters, so
+// they must agree apart from the counters advancing between the two calls. It
+// guards the accumulation loop in TimesWithContext: a mis-mapped field, a dropped
+// counter or a skipped processor group shows up as a mismatch.
+//
+// Note this does not by itself detect a revert to a GetSystemTimes-based total:
+// that call reports the sum over the calling thread's processor group, which is
+// the whole machine on a single-group host, so User/System/Idle would still match.
+// Only the Irq assertion would catch it, and only once enough interrupt time has
+// accumulated, since GetSystemTimes cannot report it at all.
+func TestTimesTotalMatchesPerCPUSum(t *testing.T) {
+	start := time.Now()
+
+	perCPU, err := Times(true)
+	require.NoError(t, err)
+	require.NotEmpty(t, perCPU)
+
+	total, err := Times(false)
+	require.NoError(t, err)
+	elapsed := time.Since(start)
+	require.Len(t, total, 1)
+	require.Equal(t, "cpu-total", total[0].CPU)
+
+	var want TimesStat
+	for _, c := range perCPU {
+		want.User += c.User
+		want.System += c.System
+		want.Idle += c.Idle
+		want.Irq += c.Irq
+	}
+
+	// Each logical CPU can advance by at most the wall clock time between the two
+	// calls, so scale the tolerance by what actually elapsed. Assuming the calls
+	// are close together would make this flaky whenever the test goroutine is
+	// descheduled on a busy CI host.
+	slack := (elapsed.Seconds() + timesTotalSlack) * float64(len(perCPU))
+	assert.InDelta(t, want.User, total[0].User, slack)
+	assert.InDelta(t, want.System, total[0].System, slack)
+	assert.InDelta(t, want.Idle, total[0].Idle, slack)
+	assert.InDelta(t, want.Irq, total[0].Irq, slack)
+}
+
+// TestSystemTimes exercises the GetSystemTimes fallback directly. TimesWithContext
+// only reaches it when perfInfo() fails, which never happens on a healthy host, so
+// without this test the fallback would ship untested.
+func TestSystemTimes(t *testing.T) {
+	total, err := systemTimes()
+	require.NoError(t, err)
+	require.Len(t, total, 1)
+
+	assert.Equal(t, "cpu-total", total[0].CPU)
+	assert.Positive(t, total[0].User)
+	assert.Positive(t, total[0].Idle)
+	assert.GreaterOrEqual(t, total[0].System, 0.0)
+	// GetSystemTimes reports no interrupt time, so Irq stays unset here. This is
+	// the one field that differs from the perfInfo-based total.
+	assert.Zero(t, total[0].Irq)
+
+	// The counters are cumulative since boot, so a second call must not go
+	// backwards. This catches a broken FILETIME recombination, which would
+	// otherwise only show up as an implausible absolute value.
+	again, err := systemTimes()
+	require.NoError(t, err)
+	require.Len(t, again, 1)
+	assert.GreaterOrEqual(t, again[0].User, total[0].User)
+	assert.GreaterOrEqual(t, again[0].System, total[0].System)
+	assert.GreaterOrEqual(t, again[0].Idle, total[0].Idle)
+}