Re: [PATCH] perf/x86: Fix open counting event error

[Ingo Molnar <mingo@kernel.org>]

* Liang, Kan <kan.liang@linux.intel.com> wrote:

> 
> 
> On 2025-04-24 12:20 p.m., Ingo Molnar wrote:
> > 
> > * Luo Gengkun <luogengkun@huaweicloud.com> wrote:
> > 
> >> Perf doesn't work at perf stat for hardware events:
> >>
> >>  $perf stat -- sleep 1
> >>  Performance counter stats for 'sleep 1':
> >>              16.44 msec task-clock                       #    0.016 CPUs utilized
> >>                  2      context-switches                 #  121.691 /sec
> >>                  0      cpu-migrations                   #    0.000 /sec
> >>                 54      page-faults                      #    3.286 K/sec
> >>    <not supported>	cycles
> >>    <not supported>	instructions
> >>    <not supported>	branches
> >>    <not supported>	branch-misses
> >>
> >> The reason is that the check in x86_pmu_hw_config for sampling event is
> >> unexpectedly applied to the counting event.
> >>
> >> Fixes: 88ec7eedbbd2 ("perf/x86: Fix low freqency setting issue")
> >> Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com>
> >> ---
> >>  arch/x86/events/core.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> >> index 6866cc5acb0b..3a4f031d2f44 100644
> >> --- a/arch/x86/events/core.c
> >> +++ b/arch/x86/events/core.c
> >> @@ -629,7 +629,7 @@ int x86_pmu_hw_config(struct perf_event *event)
> >>  	if (event->attr.type == event->pmu->type)
> >>  		event->hw.config |= x86_pmu_get_event_config(event);
> >>  
> >> -	if (!event->attr.freq && x86_pmu.limit_period) {
> >> +	if (is_sampling_event(event) && !event->attr.freq && x86_pmu.limit_period) {
> > 
> > Hm, so how come it works here, on an affected x86 system:
> > 
> > $ perf stat -- sleep 1
> > 
> >  Performance counter stats for 'sleep 1':
> > 
> >               0.64 msec task-clock:u                     #    0.001 CPUs utilized             
> >                  0      context-switches:u               #    0.000 /sec                      
> >                  0      cpu-migrations:u                 #    0.000 /sec                      
> >                 73      page-faults:u                    #  114.063 K/sec                     
> >            325,849      instructions:u                   #    0.56  insn per cycle            
> >                                                   #    0.88  stalled cycles per insn   
> >            580,323      cycles:u                         #    0.907 GHz                       
> >            286,348      stalled-cycles-frontend:u        #   49.34% frontend cycles idle      
> >             72,623      branches:u                       #  113.474 M/sec                     
> >              4,713      branch-misses:u                  #    6.49% of all branches           
> > 
> > 
> > ?
> 
> It doesn't affect all X86 platforms. It should only impact the platforms
> with limit_period used for the non-pebs events. For Intel platforms, it
> should only impact some older platforms, e.g., HSW, BDW and NHM.
> 
> For other platforms, the x86_pmu.limit_period is invoked. But the left
> is not updated. So it still equals to event->attr.sample_period.
> It doesn't error out.
> 
> 	if (!event->attr.freq && x86_pmu.limit_period) {
> 		s64 left = event->attr.sample_period;
> 		x86_pmu.limit_period(event, &left);
> 		if (left > event->attr.sample_period)
> 			return -EINVAL;
> 	}

Makes sense. I've added this paragraph to the changelog:

    It should only impact x86 platforms with limit_period used for non-PEBS
    events. For Intel platforms, it should only impact some older platforms,
    e.g., HSW, BDW and NHM.

Thanks,

	Ingo