[PATCH 0/3] tools/power turbostat: Fix segfault and restart loop issues

[PATCH 0/3] tools/power turbostat: Fix segfault and restart loop issues

[David Arcari]

This patch series addresses three stability issues found in turbostat,
primarily affecting virtualized environments where specific hardware
counters (like APERF) or MSR access may be restricted or inconsistent.

Patch 1 fixes a segmentation fault caused by referencing a NULL
fd_instr_count_percpu pointer. This occurs on systems where has_aperf
is 0 (preventing allocation), but has_aperf_access is 1 (allowing the
function call).

Patch 2 prevents an infinite loop when turbostat attempts to restart.
It ensures the restart limit is checked in all error paths and removes
logic that incorrectly reset the retry counter.

Patch 3 allows turbostat to proceed gracefully when APERF is unavailable.
Previously, the missing counter caused delta_thread() to error out,
triggering the restart mechanism.


David Arcari (3):
  tools/power turbostat: avoid segfault referencing
    fd_instr_count_percpu
  tools/power turbostat: avoid an infinite loop of restarts
  tools/power turbostat: allow turbostat to work when aperf is not
    available

 tools/power/x86/turbostat/turbostat.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

-- 
2.51.0

[PATCH 1/3] tools/power turbostat: avoid segfault referencing fd_instr_count_percpu

[David Arcari]

The problem is that fd_instr_count_percpu is allocated based on
the value of has_aperf. If has_aperf=0 then fd_instr_count_percpu
remains NULL. However, get_instr_count_fd() is called from
turbostat_init() based on the value of has_aperf_access.

On some VM systems has_aperf can be 0, while has_aperf_access can be
1.  In order to resolve the issue simply check for to see if
fd_instr_count_percpu is NULL and return -1 if it is.  Accordingly,
the has_aperf_access check can be removed from turbostat_init.

Signed-off-by: David Arcari <darcari@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 tools/power/x86/turbostat/turbostat.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index f2512d78bcbd..584b0f7f9067 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -2463,6 +2463,9 @@ static long open_perf_counter(int cpu, unsigned int type, unsigned int config, i
 
 int get_instr_count_fd(int cpu)
 {
+	if (!fd_instr_count_percpu)
+		return -1;
+
 	if (fd_instr_count_percpu[cpu])
 		return fd_instr_count_percpu[cpu];
 
@@ -10027,7 +10030,7 @@ void turbostat_init()
 	for_all_cpus(get_cpu_type, ODD_COUNTERS);
 	for_all_cpus(get_cpu_type, EVEN_COUNTERS);
 
-	if (BIC_IS_ENABLED(BIC_IPC) && has_aperf_access && get_instr_count_fd(base_cpu) != -1)
+	if (BIC_IS_ENABLED(BIC_IPC) && get_instr_count_fd(base_cpu) != -1)
 		BIC_PRESENT(BIC_IPC);
 
 	/*
-- 
2.51.0

[PATCH 2/3] tools/power turbostat: avoid an infinite loop of restarts

[David Arcari]

There are some error cases where turbostat will attempt to reinitialize
by calling the re_initialize() function.  The code attempts to avoid
an infinite loop by checking the value of 'restarted' in one case, but
not others. It should be checked in all cases of restart.  Additonally,
the 'restarted' is reset to zero at the start of the loop which also
needs to be removed.

Signed-off-by: David Arcari <darcari@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 tools/power/x86/turbostat/turbostat.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 584b0f7f9067..5567b9ecd516 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -6722,7 +6722,11 @@ void turbostat_loop()
 	set_my_sched_priority(-20);
 
 restart:
-	restarted++;
+	if (restarted++ > 10) {
+		if (!retval)
+			retval = -1;
+		exit(retval);
+	}
 
 	snapshot_proc_sysfs_files();
 	retval = for_all_cpus(get_counters, EVEN_COUNTERS);
@@ -6730,13 +6734,9 @@ void turbostat_loop()
 	if (retval < -1) {
 		exit(retval);
 	} else if (retval == -1) {
-		if (restarted > 10) {
-			exit(retval);
-		}
 		re_initialize();
 		goto restart;
 	}
-	restarted = 0;
 	done_iters = 0;
 	gettimeofday(&tv_even, (struct timezone *)NULL);
 
-- 
2.51.0

[PATCH 3/3] tools/power turbostat: allow turbostat to work when aperf is not available

[David Arcari]

Currently when aperf is not available the function has_amperf() still
returns true.  The end result is that the program gets an error in
delta_thread() which causes turbostat to restart.  We can avoid this
by not setting msr_counter_arch_infos[MSR_ARCH_INFO_APERF_INDEX].present
when aperf is not available allowing turbostat to execute normally.

Signed-off-by: David Arcari <darcari@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 tools/power/x86/turbostat/turbostat.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 5567b9ecd516..b3f1e4ae5813 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -8592,6 +8592,10 @@ void msr_perf_init_(void)
 				continue;
 
 			if (cai->needed) {
+				/* check to see if APERF is available */
+				if (cidx == MSR_ARCH_INFO_APERF_INDEX && !has_aperf)
+					continue;
+
 				/* Use perf API for this counter */
 				if (add_msr_perf_counter(cpu, cci, cai) != -1) {
 					cci->source[cai->rci_index] = COUNTER_SOURCE_PERF;
-- 
2.51.0

Re: [PATCH 1/3] tools/power turbostat: avoid segfault referencing fd_instr_count_percpu

[Len Brown]

not your fault, but looking at this code, it seems that
get_instr_count_fd(base_cpu)
assumes that 0 is an invalid FD.  Fine, but based on that you'd think
we'd use zero for invalid
and non-zero for valid as return for the function call...

On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
>
> The problem is that fd_instr_count_percpu is allocated based on
> the value of has_aperf. If has_aperf=0 then fd_instr_count_percpu
> remains NULL. However, get_instr_count_fd() is called from
> turbostat_init() based on the value of has_aperf_access.
>
> On some VM systems has_aperf can be 0, while has_aperf_access can be
> 1.  In order to resolve the issue simply check for to see if
> fd_instr_count_percpu is NULL and return -1 if it is.  Accordingly,
> the has_aperf_access check can be removed from turbostat_init.
>
> Signed-off-by: David Arcari <darcari@redhat.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-kernel@vger.kernel.org
> ---
>  tools/power/x86/turbostat/turbostat.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index f2512d78bcbd..584b0f7f9067 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -2463,6 +2463,9 @@ static long open_perf_counter(int cpu, unsigned int type, unsigned int config, i
>
>  int get_instr_count_fd(int cpu)
>  {
> +       if (!fd_instr_count_percpu)
> +               return -1;
> +
>         if (fd_instr_count_percpu[cpu])
>                 return fd_instr_count_percpu[cpu];
>
> @@ -10027,7 +10030,7 @@ void turbostat_init()
>         for_all_cpus(get_cpu_type, ODD_COUNTERS);
>         for_all_cpus(get_cpu_type, EVEN_COUNTERS);
>
> -       if (BIC_IS_ENABLED(BIC_IPC) && has_aperf_access && get_instr_count_fd(base_cpu) != -1)
> +       if (BIC_IS_ENABLED(BIC_IPC) && get_instr_count_fd(base_cpu) != -1)
>                 BIC_PRESENT(BIC_IPC);
>
>         /*
> --
> 2.51.0
>
>


-- 
Len Brown, Intel Open Source Technology Center

Re: [PATCH 2/3] tools/power turbostat: avoid an infinite loop of restarts

[Len Brown]

this patch introduces a limit of 10-restarts per turbostat lifetime,
down from infinity.

some turbostat invocations span multiple uses of cpu online/offline --
so this limit will not fly.

On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
>
> There are some error cases where turbostat will attempt to reinitialize
> by calling the re_initialize() function.  The code attempts to avoid
> an infinite loop by checking the value of 'restarted' in one case, but
> not others. It should be checked in all cases of restart.  Additonally,
> the 'restarted' is reset to zero at the start of the loop which also
> needs to be removed.
>
> Signed-off-by: David Arcari <darcari@redhat.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-kernel@vger.kernel.org
> ---
>  tools/power/x86/turbostat/turbostat.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 584b0f7f9067..5567b9ecd516 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -6722,7 +6722,11 @@ void turbostat_loop()
>         set_my_sched_priority(-20);
>
>  restart:
> -       restarted++;
> +       if (restarted++ > 10) {
> +               if (!retval)
> +                       retval = -1;
> +               exit(retval);
> +       }
>
>         snapshot_proc_sysfs_files();
>         retval = for_all_cpus(get_counters, EVEN_COUNTERS);
> @@ -6730,13 +6734,9 @@ void turbostat_loop()
>         if (retval < -1) {
>                 exit(retval);
>         } else if (retval == -1) {
> -               if (restarted > 10) {
> -                       exit(retval);
> -               }
>                 re_initialize();
>                 goto restart;
>         }
> -       restarted = 0;
>         done_iters = 0;
>         gettimeofday(&tv_even, (struct timezone *)NULL);
>
> --
> 2.51.0
>
>


-- 
Len Brown, Intel Open Source Technology Center

Re: [PATCH 3/3] tools/power turbostat: allow turbostat to work when aperf is not available

[Len Brown]

It would be helpful if you could describe exactly what environment you
are running in.

are there any MSRs?
Is APERF available via perf, but not via MSR?
etc.

On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
>
> Currently when aperf is not available the function has_amperf() still
> returns true.  The end result is that the program gets an error in
> delta_thread() which causes turbostat to restart.  We can avoid this
> by not setting msr_counter_arch_infos[MSR_ARCH_INFO_APERF_INDEX].present
> when aperf is not available allowing turbostat to execute normally.
>
> Signed-off-by: David Arcari <darcari@redhat.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: linux-kernel@vger.kernel.org
> ---
>  tools/power/x86/turbostat/turbostat.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 5567b9ecd516..b3f1e4ae5813 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -8592,6 +8592,10 @@ void msr_perf_init_(void)
>                                 continue;
>
>                         if (cai->needed) {
> +                               /* check to see if APERF is available */
> +                               if (cidx == MSR_ARCH_INFO_APERF_INDEX && !has_aperf)
> +                                       continue;
> +
>                                 /* Use perf API for this counter */
>                                 if (add_msr_perf_counter(cpu, cci, cai) != -1) {
>                                         cci->source[cai->rci_index] = COUNTER_SOURCE_PERF;
> --
> 2.51.0
>
>


-- 
Len Brown, Intel Open Source Technology Center

Re: [PATCH 2/3] tools/power turbostat: avoid an infinite loop of restarts

[David Arcari]

I see.  Perhaps this isn't fixable then.  I'll take another look.

-DA

On 11/25/25 2:12 PM, Len Brown wrote:
> this patch introduces a limit of 10-restarts per turbostat lifetime,
> down from infinity.
> 
> some turbostat invocations span multiple uses of cpu online/offline --
> so this limit will not fly.
> 
> On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
>>
>> There are some error cases where turbostat will attempt to reinitialize
>> by calling the re_initialize() function.  The code attempts to avoid
>> an infinite loop by checking the value of 'restarted' in one case, but
>> not others. It should be checked in all cases of restart.  Additonally,
>> the 'restarted' is reset to zero at the start of the loop which also
>> needs to be removed.
>>
>> Signed-off-by: David Arcari <darcari@redhat.com>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   tools/power/x86/turbostat/turbostat.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
>> index 584b0f7f9067..5567b9ecd516 100644
>> --- a/tools/power/x86/turbostat/turbostat.c
>> +++ b/tools/power/x86/turbostat/turbostat.c
>> @@ -6722,7 +6722,11 @@ void turbostat_loop()
>>          set_my_sched_priority(-20);
>>
>>   restart:
>> -       restarted++;
>> +       if (restarted++ > 10) {
>> +               if (!retval)
>> +                       retval = -1;
>> +               exit(retval);
>> +       }
>>
>>          snapshot_proc_sysfs_files();
>>          retval = for_all_cpus(get_counters, EVEN_COUNTERS);
>> @@ -6730,13 +6734,9 @@ void turbostat_loop()
>>          if (retval < -1) {
>>                  exit(retval);
>>          } else if (retval == -1) {
>> -               if (restarted > 10) {
>> -                       exit(retval);
>> -               }
>>                  re_initialize();
>>                  goto restart;
>>          }
>> -       restarted = 0;
>>          done_iters = 0;
>>          gettimeofday(&tv_even, (struct timezone *)NULL);
>>
>> --
>> 2.51.0
>>
>>
> 
> 

Re: [PATCH 3/3] tools/power turbostat: allow turbostat to work when aperf is not available

[David Arcari]

On 11/25/25 2:14 PM, Len Brown wrote:
> It would be helpful if you could describe exactly what environment you
> are running in.

It' a VMWARE instance.

CPUID(0): AuthenticAMD 0xd CPUID levels
CPUID(1): family:model:stepping 0x17:0:0 (23:0:0) microcode 0x0
CPUID(0x80000000): max_extended_levels: 0x8000001f
CPUID(1): SSE3 - - - - TSC MSR - - -
CPUID(6): No-APERF, No-TURBO, No-DTS, No-PTM, No-HWP, No-HWPnotify, 
No-HWPwindow, No-HWPepp, No-HWPpkg, No-EPB
CPUID(7): No-SGX No-Hybrid


> 
> are there any MSRs?

I'm not certain, is there something in particular you are looking for?

> Is APERF available via perf, but not via MSR?
> etc.

I don't believe that APERF was available via perf. I'll go back and 
verify when I have a chance.

-DA

> 
> On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
>>
>> Currently when aperf is not available the function has_amperf() still
>> returns true.  The end result is that the program gets an error in
>> delta_thread() which causes turbostat to restart.  We can avoid this
>> by not setting msr_counter_arch_infos[MSR_ARCH_INFO_APERF_INDEX].present
>> when aperf is not available allowing turbostat to execute normally.
>>
>> Signed-off-by: David Arcari <darcari@redhat.com>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   tools/power/x86/turbostat/turbostat.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
>> index 5567b9ecd516..b3f1e4ae5813 100644
>> --- a/tools/power/x86/turbostat/turbostat.c
>> +++ b/tools/power/x86/turbostat/turbostat.c
>> @@ -8592,6 +8592,10 @@ void msr_perf_init_(void)
>>                                  continue;
>>
>>                          if (cai->needed) {
>> +                               /* check to see if APERF is available */
>> +                               if (cidx == MSR_ARCH_INFO_APERF_INDEX && !has_aperf)
>> +                                       continue;
>> +
>>                                  /* Use perf API for this counter */
>>                                  if (add_msr_perf_counter(cpu, cci, cai) != -1) {
>>                                          cci->source[cai->rci_index] = COUNTER_SOURCE_PERF;
>> --
>> 2.51.0
>>
>>
> 
> 

Re: [PATCH 2/3] tools/power turbostat: avoid an infinite loop of restarts

[Len Brown]

I guess it is sort of an art form upon malfunction to decide whether
it is fatal or not.

I suppose if we know that a counter used to work, and it stops working, then we
are responding to a configuration change.  We don't want to count a
certain number
of configuration changes as a fatal error.

A more useful heuristic may be to recognize when there are failures
without any intervening successes,
and if those happen many times per second.  Likely that is the
infinite loop case we're looking for...

On Tue, Nov 25, 2025 at 4:51 PM David Arcari <darcari@redhat.com> wrote:
>
>
> I see.  Perhaps this isn't fixable then.  I'll take another look.
>
> -DA
>
> On 11/25/25 2:12 PM, Len Brown wrote:
> > this patch introduces a limit of 10-restarts per turbostat lifetime,
> > down from infinity.
> >
> > some turbostat invocations span multiple uses of cpu online/offline --
> > so this limit will not fly.
> >
> > On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
> >>
> >> There are some error cases where turbostat will attempt to reinitialize
> >> by calling the re_initialize() function.  The code attempts to avoid
> >> an infinite loop by checking the value of 'restarted' in one case, but
> >> not others. It should be checked in all cases of restart.  Additonally,
> >> the 'restarted' is reset to zero at the start of the loop which also
> >> needs to be removed.
> >>
> >> Signed-off-by: David Arcari <darcari@redhat.com>
> >> Cc: Len Brown <lenb@kernel.org>
> >> Cc: linux-kernel@vger.kernel.org
> >> ---
> >>   tools/power/x86/turbostat/turbostat.c | 10 +++++-----
> >>   1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> >> index 584b0f7f9067..5567b9ecd516 100644
> >> --- a/tools/power/x86/turbostat/turbostat.c
> >> +++ b/tools/power/x86/turbostat/turbostat.c
> >> @@ -6722,7 +6722,11 @@ void turbostat_loop()
> >>          set_my_sched_priority(-20);
> >>
> >>   restart:
> >> -       restarted++;
> >> +       if (restarted++ > 10) {
> >> +               if (!retval)
> >> +                       retval = -1;
> >> +               exit(retval);
> >> +       }
> >>
> >>          snapshot_proc_sysfs_files();
> >>          retval = for_all_cpus(get_counters, EVEN_COUNTERS);
> >> @@ -6730,13 +6734,9 @@ void turbostat_loop()
> >>          if (retval < -1) {
> >>                  exit(retval);
> >>          } else if (retval == -1) {
> >> -               if (restarted > 10) {
> >> -                       exit(retval);
> >> -               }
> >>                  re_initialize();
> >>                  goto restart;
> >>          }
> >> -       restarted = 0;
> >>          done_iters = 0;
> >>          gettimeofday(&tv_even, (struct timezone *)NULL);
> >>
> >> --
> >> 2.51.0
> >>
> >>
> >
> >
>


-- 
Len Brown, Intel Open Source Technology Center

Re: [PATCH 2/3] tools/power turbostat: avoid an infinite loop of restarts

[David Arcari]

I'll take a look with that in mind, but this is really a second order 
issue and could be deferred until a latter date.  It's patches 1 and 3 
that resolve the main issue and should be resolved more quickly.

-DA

On 11/27/25 12:45 PM, Len Brown wrote:
> I guess it is sort of an art form upon malfunction to decide whether
> it is fatal or not.
> 
> I suppose if we know that a counter used to work, and it stops working, then we
> are responding to a configuration change.  We don't want to count a
> certain number
> of configuration changes as a fatal error.
> 
> A more useful heuristic may be to recognize when there are failures
> without any intervening successes,
> and if those happen many times per second.  Likely that is the
> infinite loop case we're looking for...
> 
> On Tue, Nov 25, 2025 at 4:51 PM David Arcari <darcari@redhat.com> wrote:
>>
>>
>> I see.  Perhaps this isn't fixable then.  I'll take another look.
>>
>> -DA
>>
>> On 11/25/25 2:12 PM, Len Brown wrote:
>>> this patch introduces a limit of 10-restarts per turbostat lifetime,
>>> down from infinity.
>>>
>>> some turbostat invocations span multiple uses of cpu online/offline --
>>> so this limit will not fly.
>>>
>>> On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
>>>>
>>>> There are some error cases where turbostat will attempt to reinitialize
>>>> by calling the re_initialize() function.  The code attempts to avoid
>>>> an infinite loop by checking the value of 'restarted' in one case, but
>>>> not others. It should be checked in all cases of restart.  Additonally,
>>>> the 'restarted' is reset to zero at the start of the loop which also
>>>> needs to be removed.
>>>>
>>>> Signed-off-by: David Arcari <darcari@redhat.com>
>>>> Cc: Len Brown <lenb@kernel.org>
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>>    tools/power/x86/turbostat/turbostat.c | 10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
>>>> index 584b0f7f9067..5567b9ecd516 100644
>>>> --- a/tools/power/x86/turbostat/turbostat.c
>>>> +++ b/tools/power/x86/turbostat/turbostat.c
>>>> @@ -6722,7 +6722,11 @@ void turbostat_loop()
>>>>           set_my_sched_priority(-20);
>>>>
>>>>    restart:
>>>> -       restarted++;
>>>> +       if (restarted++ > 10) {
>>>> +               if (!retval)
>>>> +                       retval = -1;
>>>> +               exit(retval);
>>>> +       }
>>>>
>>>>           snapshot_proc_sysfs_files();
>>>>           retval = for_all_cpus(get_counters, EVEN_COUNTERS);
>>>> @@ -6730,13 +6734,9 @@ void turbostat_loop()
>>>>           if (retval < -1) {
>>>>                   exit(retval);
>>>>           } else if (retval == -1) {
>>>> -               if (restarted > 10) {
>>>> -                       exit(retval);
>>>> -               }
>>>>                   re_initialize();
>>>>                   goto restart;
>>>>           }
>>>> -       restarted = 0;
>>>>           done_iters = 0;
>>>>           gettimeofday(&tv_even, (struct timezone *)NULL);
>>>>
>>>> --
>>>> 2.51.0
>>>>
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH 1/3] tools/power turbostat: avoid segfault referencing fd_instr_count_percpu

[David Arcari]

So get_instr_count_fd() calls open_perf_counter() which in turn calls 
perf_event_open() which returns the value from syscall().  From the 
documentation this seems to return -1 in the case of a failure.

Looking at get_instr_count_fd() I see:

int get_instr_count_fd(int cpu)
{
	if (fd_instr_count_percpu[cpu])
		return fd_instr_count_percpu[cpu];

	fd_instr_count_percpu[cpu] = open_perf_counter(cpu, PERF_TYPE_HARDWARE, 
PERF_COUNT_HW_INSTRUCTIONS, -1, 0);

	return fd_instr_count_percpu[cpu];
}


So open_perf_counter() is only called when fd_instr_count_percpu[cpu] is 
0.  In that case the return value is stored in 
fd_instr_count_percpu[cpu].  So in the case of an error this value would 
be -1; otherwise, it should be a valid file descriptor.  In fact, I 
don't think the function should ever return 0.

As far as I can tell fd_instr_count_percpu[] is initialized to zero so 
that  get_instr_count_fd() can discern whether or not 
open_perf_counter() needs to be called.

Am I missing something?

I do see that free_fd_instr_count_percpu() has a bug as I think the code 
should be:

		if (fd_instr_count_percpu[i] > 0)

instead of:

		if (fd_instr_count_percpu[i] != 0)


Thanks,
-DA

On 11/25/25 2:11 PM, Len Brown wrote:
> not your fault, but looking at this code, it seems that
> get_instr_count_fd(base_cpu)
> assumes that 0 is an invalid FD.  Fine, but based on that you'd think
> we'd use zero for invalid
> and non-zero for valid as return for the function call...
> 
> On Tue, Nov 18, 2025 at 10:58 AM David Arcari <darcari@redhat.com> wrote:
>>
>> The problem is that fd_instr_count_percpu is allocated based on
>> the value of has_aperf. If has_aperf=0 then fd_instr_count_percpu
>> remains NULL. However, get_instr_count_fd() is called from
>> turbostat_init() based on the value of has_aperf_access.
>>
>> On some VM systems has_aperf can be 0, while has_aperf_access can be
>> 1.  In order to resolve the issue simply check for to see if
>> fd_instr_count_percpu is NULL and return -1 if it is.  Accordingly,
>> the has_aperf_access check can be removed from turbostat_init.
>>
>> Signed-off-by: David Arcari <darcari@redhat.com>
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   tools/power/x86/turbostat/turbostat.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
>> index f2512d78bcbd..584b0f7f9067 100644
>> --- a/tools/power/x86/turbostat/turbostat.c
>> +++ b/tools/power/x86/turbostat/turbostat.c
>> @@ -2463,6 +2463,9 @@ static long open_perf_counter(int cpu, unsigned int type, unsigned int config, i
>>
>>   int get_instr_count_fd(int cpu)
>>   {
>> +       if (!fd_instr_count_percpu)
>> +               return -1;
>> +
>>          if (fd_instr_count_percpu[cpu])
>>                  return fd_instr_count_percpu[cpu];
>>
>> @@ -10027,7 +10030,7 @@ void turbostat_init()
>>          for_all_cpus(get_cpu_type, ODD_COUNTERS);
>>          for_all_cpus(get_cpu_type, EVEN_COUNTERS);
>>
>> -       if (BIC_IS_ENABLED(BIC_IPC) && has_aperf_access && get_instr_count_fd(base_cpu) != -1)
>> +       if (BIC_IS_ENABLED(BIC_IPC) && get_instr_count_fd(base_cpu) != -1)
>>                  BIC_PRESENT(BIC_IPC);
>>
>>          /*
>> --
>> 2.51.0
>>
>>
> 
> 

Re: [PATCH 3/3] tools/power turbostat: allow turbostat to work when aperf is not available

[David Arcari]

On 11/25/25 4:55 PM, David Arcari wrote:
> 
> 
> On 11/25/25 2:14 PM, Len Brown wrote:
>> It would be helpful if you could describe exactly what environment you
>> are running in.
> 
> It' a VMWARE instance.
> 
> CPUID(0): AuthenticAMD 0xd CPUID levels
> CPUID(1): family:model:stepping 0x17:0:0 (23:0:0) microcode 0x0
> CPUID(0x80000000): max_extended_levels: 0x8000001f
> CPUID(1): SSE3 - - - - TSC MSR - - -
> CPUID(6): No-APERF, No-TURBO, No-DTS, No-PTM, No-HWP, No-HWPnotify, No- 
> HWPwindow, No-HWPepp, No-HWPpkg, No-EPB
> CPUID(7): No-SGX No-Hybrid
> 
> 
>>
>> are there any MSRs?
> 
> I'm not certain, is there something in particular you are looking for?
> 

rdmsr returns zero for MSR_IA32_APERF and MSR_IA32_MPERF.

>> Is APERF available via perf, but not via MSR?
>> etc.

No.  add_msr_perf_counter() returns -1 as the call to open_perf_counter 
returns -1.

> 
> I don't believe that APERF was available via perf. I'll go back and 
> verify when I have a chance.
> 
> -DA
> 
Is there anything else you need?

Thanks,
-DA

Re: [PATCH 3/3] tools/power turbostat: allow turbostat to work when aperf is not available

[Len Brown]

> ...add_msr_perf_counter() returns -1

good.  and then add_msr_counter() succeeds because...

> rdmsr returns zero for MSR_IA32_APERF and MSR_IA32_MPERF.

Your patch is a good suggestion -- though it checks for APERF only and
not for MPERF.

We already ran CPUID and cleared has_aperf, so I'm thinking we should
be heading this off earlier.  Let me send you a test patch later today.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 3/3] tools/power turbostat: allow turbostat to work when aperf is not available

[Len Brown]

Something like this?

diff --git a/tools/power/x86/turbostat/turbostat.c
b/tools/power/x86/turbostat/turbostat.c
index 5bc47ad5da09..4a847e7e9c65 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -8530,7 +8530,7 @@ void rapl_perf_init(void)
 /* Assumes msr_counter_info is populated */
 static int has_amperf_access(void)
 {
-       return msr_counter_arch_infos[MSR_ARCH_INFO_APERF_INDEX].present &&
+       return has_aperf &&
msr_counter_arch_infos[MSR_ARCH_INFO_APERF_INDEX].present &&
            msr_counter_arch_infos[MSR_ARCH_INFO_MPERF_INDEX].present;
 }


On Mon, Dec 1, 2025 at 2:53 PM Len Brown <lenb@kernel.org> wrote:
>
> > ...add_msr_perf_counter() returns -1
>
> good.  and then add_msr_counter() succeeds because...
>
> > rdmsr returns zero for MSR_IA32_APERF and MSR_IA32_MPERF.
>
> Your patch is a good suggestion -- though it checks for APERF only and
> not for MPERF.
>
> We already ran CPUID and cleared has_aperf, so I'm thinking we should
> be heading this off earlier.  Let me send you a test patch later today.
>
> thanks,
> Len Brown, Intel Open Source Technology Center



-- 
Len Brown, Intel Open Source Technology Center

Re: [PATCH 3/3] tools/power turbostat: allow turbostat to work when aperf is not available

[David Arcari]

On 12/1/25 3:11 PM, Len Brown wrote:
> Something like this?

Yes - this works.  It's actually nice and clean and actually mitigates 
the need for patch 0001.  Clearly an improvement over my patch 3.

How would you like to proceed?  I could resubmit a single v2 patch (with 
an appropriate explanation).

I'd rather not attempt to solve the retry issue.  It's really a second 
order issue and I don't think I'm the appropriate person to do that work.

-DA

> 
> diff --git a/tools/power/x86/turbostat/turbostat.c
> b/tools/power/x86/turbostat/turbostat.c
> index 5bc47ad5da09..4a847e7e9c65 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -8530,7 +8530,7 @@ void rapl_perf_init(void)
>   /* Assumes msr_counter_info is populated */
>   static int has_amperf_access(void)
>   {
> -       return msr_counter_arch_infos[MSR_ARCH_INFO_APERF_INDEX].present &&
> +       return has_aperf &&
> msr_counter_arch_infos[MSR_ARCH_INFO_APERF_INDEX].present &&
>              msr_counter_arch_infos[MSR_ARCH_INFO_MPERF_INDEX].present;
>   }
> 
> 
> On Mon, Dec 1, 2025 at 2:53 PM Len Brown <lenb@kernel.org> wrote:
>>
>>> ...add_msr_perf_counter() returns -1
>>
>> good.  and then add_msr_counter() succeeds because...
>>
>>> rdmsr returns zero for MSR_IA32_APERF and MSR_IA32_MPERF.
>>
>> Your patch is a good suggestion -- though it checks for APERF only and
>> not for MPERF.
>>
>> We already ran CPUID and cleared has_aperf, so I'm thinking we should
>> be heading this off earlier.  Let me send you a test patch later today.
>>
>> thanks,
>> Len Brown, Intel Open Source Technology Center
> 
> 
> 

Re: [PATCH 3/3] tools/power turbostat: allow turbostat to work when aperf is not available

[Len Brown]

Thanks for testing.  I'll send a final patch today.

-L

On Tue, Dec 2, 2025 at 7:55 AM David Arcari <darcari@redhat.com> wrote:
>
>
>
> On 12/1/25 3:11 PM, Len Brown wrote:
> > Something like this?
>
> Yes - this works.  It's actually nice and clean and actually mitigates
> the need for patch 0001.  Clearly an improvement over my patch 3.
>
> How would you like to proceed?  I could resubmit a single v2 patch (with
> an appropriate explanation).
>
> I'd rather not attempt to solve the retry issue.  It's really a second
> order issue and I don't think I'm the appropriate person to do that work.
>
> -DA
>
> >
> > diff --git a/tools/power/x86/turbostat/turbostat.c
> > b/tools/power/x86/turbostat/turbostat.c
> > index 5bc47ad5da09..4a847e7e9c65 100644
> > --- a/tools/power/x86/turbostat/turbostat.c
> > +++ b/tools/power/x86/turbostat/turbostat.c
> > @@ -8530,7 +8530,7 @@ void rapl_perf_init(void)
> >   /* Assumes msr_counter_info is populated */
> >   static int has_amperf_access(void)
> >   {
> > -       return msr_counter_arch_infos[MSR_ARCH_INFO_APERF_INDEX].present &&
> > +       return has_aperf &&
> > msr_counter_arch_infos[MSR_ARCH_INFO_APERF_INDEX].present &&
> >              msr_counter_arch_infos[MSR_ARCH_INFO_MPERF_INDEX].present;
> >   }
> >
> >
> > On Mon, Dec 1, 2025 at 2:53 PM Len Brown <lenb@kernel.org> wrote:
> >>
> >>> ...add_msr_perf_counter() returns -1
> >>
> >> good.  and then add_msr_counter() succeeds because...
> >>
> >>> rdmsr returns zero for MSR_IA32_APERF and MSR_IA32_MPERF.
> >>
> >> Your patch is a good suggestion -- though it checks for APERF only and
> >> not for MPERF.
> >>
> >> We already ran CPUID and cleared has_aperf, so I'm thinking we should
> >> be heading this off earlier.  Let me send you a test patch later today.
> >>
> >> thanks,
> >> Len Brown, Intel Open Source Technology Center
> >
> >
> >
>
>


-- 
Len Brown, Intel Open Source Technology Center