linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] perf: Fix 0 count issue of cpu-clock
@ 2025-11-12  8:05 Dapeng Mi
  2025-11-12 16:42 ` Ian Rogers
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Dapeng Mi @ 2025-11-12  8:05 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
	Andi Kleen, Eranian Stephane
  Cc: linux-kernel, linux-perf-users, Dapeng Mi, Zide Chen,
	Falcon Thomas, Xudong Hao, Dapeng Mi

Currently cpu-clock event always returns 0 count, e.g.,

perf stat -e cpu-clock -- sleep 1

 Performance counter stats for 'sleep 1':
                 0      cpu-clock                        #    0.000 CPUs utilized
       1.002308394 seconds time elapsed

The root cause is the commit 'bc4394e5e79c ("perf: Fix the throttle
 error of some clock events")' adds PERF_EF_UPDATE flag check before
calling cpu_clock_event_update() to update the count, however the
PERF_EF_UPDATE flag is never set when the cpu-clock event is stopped in
counting mode (pmu->dev() -> cpu_clock_event_del() ->
cpu_clock_event_stop()). This leads to the cpu-clock event count is
never updated.

To fix this issue, force to set PERF_EF_UPDATE flag for cpu-clock event
just like what task-clock does. Besides, or flags with PERF_EF_UPDATE
for task-clock although currently the flags argument would always be 0.

Fixes: bc4394e5e79c ("perf: Fix the throttle error of some clock events")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---

With this change, both cpu-clock and task-clock can do counting and
samping correctly.

1. perf stat -e cpu-clock,task-clock -- true

 Performance counter stats for 'true':
           240,636      cpu-clock                        #    0.358 CPUs utilized
           243,319      task-clock                       #    0.362 CPUs utilized

2. perf record -e cpu-clock -c 10000 -Iax,bx -- sleep 1
[ perf record: Woken up 2 times to write data ]
[ perf record: Captured and wrote 0.028 MB perf.data (36 samples) ]

3. perf record -e task-clock -c 10000 -Iax,bx -- sleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.029 MB perf.data (41 samples) ]

 kernel/events/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index f6a08c73f783..77d3af5959c1 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11964,7 +11964,7 @@ static int cpu_clock_event_add(struct perf_event *event, int flags)
 
 static void cpu_clock_event_del(struct perf_event *event, int flags)
 {
-	cpu_clock_event_stop(event, flags);
+	cpu_clock_event_stop(event, flags | PERF_EF_UPDATE);
 }
 
 static void cpu_clock_event_read(struct perf_event *event)
@@ -12043,7 +12043,7 @@ static int task_clock_event_add(struct perf_event *event, int flags)
 
 static void task_clock_event_del(struct perf_event *event, int flags)
 {
-	task_clock_event_stop(event, PERF_EF_UPDATE);
+	task_clock_event_stop(event, flags | PERF_EF_UPDATE);
 }
 
 static void task_clock_event_read(struct perf_event *event)

base-commit: 2093d8cf80fa5552d1025a78a8f3a10bf3b6466e
prerequisite-patch-id: a15bcd62a8dcd219d17489eef88b66ea5488a2a0
prerequisite-patch-id: 2a0eefce67b21d1f30c272fd8115b0dc1aca3897
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-11-12  8:05 [PATCH] perf: Fix 0 count issue of cpu-clock Dapeng Mi
@ 2025-11-12 16:42 ` Ian Rogers
  2025-11-17 17:04   ` Ian Rogers
  2025-11-18  1:43 ` Namhyung Kim
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Ian Rogers @ 2025-11-12 16:42 UTC (permalink / raw)
  To: Dapeng Mi
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Adrian Hunter, Alexander Shishkin, Andi Kleen,
	Eranian Stephane, linux-kernel, linux-perf-users, Dapeng Mi,
	Zide Chen, Falcon Thomas, Xudong Hao

On Wed, Nov 12, 2025 at 12:07 AM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>
> Currently cpu-clock event always returns 0 count, e.g.,
>
> perf stat -e cpu-clock -- sleep 1
>
>  Performance counter stats for 'sleep 1':
>                  0      cpu-clock                        #    0.000 CPUs utilized
>        1.002308394 seconds time elapsed
>
> The root cause is the commit 'bc4394e5e79c ("perf: Fix the throttle
>  error of some clock events")' adds PERF_EF_UPDATE flag check before
> calling cpu_clock_event_update() to update the count, however the
> PERF_EF_UPDATE flag is never set when the cpu-clock event is stopped in
> counting mode (pmu->dev() -> cpu_clock_event_del() ->
> cpu_clock_event_stop()). This leads to the cpu-clock event count is
> never updated.
>
> To fix this issue, force to set PERF_EF_UPDATE flag for cpu-clock event
> just like what task-clock does. Besides, or flags with PERF_EF_UPDATE
> for task-clock although currently the flags argument would always be 0.
>
> Fixes: bc4394e5e79c ("perf: Fix the throttle error of some clock events")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

Thanks Dapeng! This is a fairly major regression and I'm surprised my
kernel picked it up so quickly. For those interested the relevant part
of the breaking change is requiring PERF_EF_UPDATE:

```
@@ -11829,7 +11834,8 @@ static void cpu_clock_event_start(struct
perf_event *event, int flags)
 static void cpu_clock_event_stop(struct perf_event *event, int flags)
 {
        perf_swevent_cancel_hrtimer(event);
-       cpu_clock_event_update(event);
+       if (flags & PERF_EF_UPDATE)
+               cpu_clock_event_update(event);
 }
 ```

Reviewed-by: Ian Rogers <irogers@google.com>

Hopefully a maintainer can pick this up quickly. Thanks,
Ian

> ---
>
> With this change, both cpu-clock and task-clock can do counting and
> samping correctly.
>
> 1. perf stat -e cpu-clock,task-clock -- true
>
>  Performance counter stats for 'true':
>            240,636      cpu-clock                        #    0.358 CPUs utilized
>            243,319      task-clock                       #    0.362 CPUs utilized
>
> 2. perf record -e cpu-clock -c 10000 -Iax,bx -- sleep 1
> [ perf record: Woken up 2 times to write data ]
> [ perf record: Captured and wrote 0.028 MB perf.data (36 samples) ]
>
> 3. perf record -e task-clock -c 10000 -Iax,bx -- sleep 1
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.029 MB perf.data (41 samples) ]
>
>  kernel/events/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f6a08c73f783..77d3af5959c1 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11964,7 +11964,7 @@ static int cpu_clock_event_add(struct perf_event *event, int flags)
>
>  static void cpu_clock_event_del(struct perf_event *event, int flags)
>  {
> -       cpu_clock_event_stop(event, flags);
> +       cpu_clock_event_stop(event, flags | PERF_EF_UPDATE);
>  }
>
>  static void cpu_clock_event_read(struct perf_event *event)
> @@ -12043,7 +12043,7 @@ static int task_clock_event_add(struct perf_event *event, int flags)
>
>  static void task_clock_event_del(struct perf_event *event, int flags)
>  {
> -       task_clock_event_stop(event, PERF_EF_UPDATE);
> +       task_clock_event_stop(event, flags | PERF_EF_UPDATE);
>  }
>
>  static void task_clock_event_read(struct perf_event *event)
>
> base-commit: 2093d8cf80fa5552d1025a78a8f3a10bf3b6466e
> prerequisite-patch-id: a15bcd62a8dcd219d17489eef88b66ea5488a2a0
> prerequisite-patch-id: 2a0eefce67b21d1f30c272fd8115b0dc1aca3897
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-11-12 16:42 ` Ian Rogers
@ 2025-11-17 17:04   ` Ian Rogers
  0 siblings, 0 replies; 11+ messages in thread
From: Ian Rogers @ 2025-11-17 17:04 UTC (permalink / raw)
  To: Dapeng Mi, Ingo Molnar, Peter Zijlstra
  Cc: Arnaldo Carvalho de Melo, Namhyung Kim, Adrian Hunter,
	Alexander Shishkin, Andi Kleen, Eranian Stephane, linux-kernel,
	linux-perf-users, Dapeng Mi, Zide Chen, Falcon Thomas, Xudong Hao

On Wed, Nov 12, 2025 at 8:42 AM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Nov 12, 2025 at 12:07 AM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
> >
> > Currently cpu-clock event always returns 0 count, e.g.,
> >
> > perf stat -e cpu-clock -- sleep 1
> >
> >  Performance counter stats for 'sleep 1':
> >                  0      cpu-clock                        #    0.000 CPUs utilized
> >        1.002308394 seconds time elapsed
> >
> > The root cause is the commit 'bc4394e5e79c ("perf: Fix the throttle
> >  error of some clock events")' adds PERF_EF_UPDATE flag check before
> > calling cpu_clock_event_update() to update the count, however the
> > PERF_EF_UPDATE flag is never set when the cpu-clock event is stopped in
> > counting mode (pmu->dev() -> cpu_clock_event_del() ->
> > cpu_clock_event_stop()). This leads to the cpu-clock event count is
> > never updated.
> >
> > To fix this issue, force to set PERF_EF_UPDATE flag for cpu-clock event
> > just like what task-clock does. Besides, or flags with PERF_EF_UPDATE
> > for task-clock although currently the flags argument would always be 0.
> >
> > Fixes: bc4394e5e79c ("perf: Fix the throttle error of some clock events")
> > Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> Thanks Dapeng! This is a fairly major regression and I'm surprised my
> kernel picked it up so quickly. For those interested the relevant part
> of the breaking change is requiring PERF_EF_UPDATE:
>
> ```
> @@ -11829,7 +11834,8 @@ static void cpu_clock_event_start(struct
> perf_event *event, int flags)
>  static void cpu_clock_event_stop(struct perf_event *event, int flags)
>  {
>         perf_swevent_cancel_hrtimer(event);
> -       cpu_clock_event_update(event);
> +       if (flags & PERF_EF_UPDATE)
> +               cpu_clock_event_update(event);
>  }
>  ```
>
> Reviewed-by: Ian Rogers <irogers@google.com>
>
> Hopefully a maintainer can pick this up quickly. Thanks,
> Ian

Hi Peter and Ingo,

any update on picking this up? Having timer based software events
(task-clock, cpu-clock) broken is fairly major regression especially
in environments like hypervisors where PMU access is blocked.

Thanks,
Ian

> > ---
> >
> > With this change, both cpu-clock and task-clock can do counting and
> > samping correctly.
> >
> > 1. perf stat -e cpu-clock,task-clock -- true
> >
> >  Performance counter stats for 'true':
> >            240,636      cpu-clock                        #    0.358 CPUs utilized
> >            243,319      task-clock                       #    0.362 CPUs utilized
> >
> > 2. perf record -e cpu-clock -c 10000 -Iax,bx -- sleep 1
> > [ perf record: Woken up 2 times to write data ]
> > [ perf record: Captured and wrote 0.028 MB perf.data (36 samples) ]
> >
> > 3. perf record -e task-clock -c 10000 -Iax,bx -- sleep 1
> > [ perf record: Woken up 1 times to write data ]
> > [ perf record: Captured and wrote 0.029 MB perf.data (41 samples) ]
> >
> >  kernel/events/core.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index f6a08c73f783..77d3af5959c1 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -11964,7 +11964,7 @@ static int cpu_clock_event_add(struct perf_event *event, int flags)
> >
> >  static void cpu_clock_event_del(struct perf_event *event, int flags)
> >  {
> > -       cpu_clock_event_stop(event, flags);
> > +       cpu_clock_event_stop(event, flags | PERF_EF_UPDATE);
> >  }
> >
> >  static void cpu_clock_event_read(struct perf_event *event)
> > @@ -12043,7 +12043,7 @@ static int task_clock_event_add(struct perf_event *event, int flags)
> >
> >  static void task_clock_event_del(struct perf_event *event, int flags)
> >  {
> > -       task_clock_event_stop(event, PERF_EF_UPDATE);
> > +       task_clock_event_stop(event, flags | PERF_EF_UPDATE);
> >  }
> >
> >  static void task_clock_event_read(struct perf_event *event)
> >
> > base-commit: 2093d8cf80fa5552d1025a78a8f3a10bf3b6466e
> > prerequisite-patch-id: a15bcd62a8dcd219d17489eef88b66ea5488a2a0
> > prerequisite-patch-id: 2a0eefce67b21d1f30c272fd8115b0dc1aca3897
> > --
> > 2.34.1
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-11-12  8:05 [PATCH] perf: Fix 0 count issue of cpu-clock Dapeng Mi
  2025-11-12 16:42 ` Ian Rogers
@ 2025-11-18  1:43 ` Namhyung Kim
  2025-11-18 10:50 ` Peter Zijlstra
  2025-11-18 11:03 ` Peter Zijlstra
  3 siblings, 0 replies; 11+ messages in thread
From: Namhyung Kim @ 2025-11-18  1:43 UTC (permalink / raw)
  To: Dapeng Mi
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Ian Rogers,
	Adrian Hunter, Alexander Shishkin, Andi Kleen, Eranian Stephane,
	linux-kernel, linux-perf-users, Dapeng Mi, Zide Chen,
	Falcon Thomas, Xudong Hao

Hello,

On Wed, Nov 12, 2025 at 04:05:26PM +0800, Dapeng Mi wrote:
> Currently cpu-clock event always returns 0 count, e.g.,
> 
> perf stat -e cpu-clock -- sleep 1
> 
>  Performance counter stats for 'sleep 1':
>                  0      cpu-clock                        #    0.000 CPUs utilized
>        1.002308394 seconds time elapsed
> 
> The root cause is the commit 'bc4394e5e79c ("perf: Fix the throttle
>  error of some clock events")' adds PERF_EF_UPDATE flag check before
> calling cpu_clock_event_update() to update the count, however the
> PERF_EF_UPDATE flag is never set when the cpu-clock event is stopped in
> counting mode (pmu->dev() -> cpu_clock_event_del() ->
> cpu_clock_event_stop()). This leads to the cpu-clock event count is
> never updated.
> 
> To fix this issue, force to set PERF_EF_UPDATE flag for cpu-clock event
> just like what task-clock does. Besides, or flags with PERF_EF_UPDATE
> for task-clock although currently the flags argument would always be 0.
> 
> Fixes: bc4394e5e79c ("perf: Fix the throttle error of some clock events")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> ---
> 
> With this change, both cpu-clock and task-clock can do counting and
> samping correctly.
> 
> 1. perf stat -e cpu-clock,task-clock -- true
> 
>  Performance counter stats for 'true':
>            240,636      cpu-clock                        #    0.358 CPUs utilized
>            243,319      task-clock                       #    0.362 CPUs utilized
> 
> 2. perf record -e cpu-clock -c 10000 -Iax,bx -- sleep 1
> [ perf record: Woken up 2 times to write data ]
> [ perf record: Captured and wrote 0.028 MB perf.data (36 samples) ]
> 
> 3. perf record -e task-clock -c 10000 -Iax,bx -- sleep 1
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.029 MB perf.data (41 samples) ]
> 
>  kernel/events/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f6a08c73f783..77d3af5959c1 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11964,7 +11964,7 @@ static int cpu_clock_event_add(struct perf_event *event, int flags)
>  
>  static void cpu_clock_event_del(struct perf_event *event, int flags)
>  {
> -	cpu_clock_event_stop(event, flags);
> +	cpu_clock_event_stop(event, flags | PERF_EF_UPDATE);
>  }
>  
>  static void cpu_clock_event_read(struct perf_event *event)
> @@ -12043,7 +12043,7 @@ static int task_clock_event_add(struct perf_event *event, int flags)
>  
>  static void task_clock_event_del(struct perf_event *event, int flags)
>  {
> -	task_clock_event_stop(event, PERF_EF_UPDATE);
> +	task_clock_event_stop(event, flags | PERF_EF_UPDATE);
>  }
>  
>  static void task_clock_event_read(struct perf_event *event)
> 
> base-commit: 2093d8cf80fa5552d1025a78a8f3a10bf3b6466e
> prerequisite-patch-id: a15bcd62a8dcd219d17489eef88b66ea5488a2a0
> prerequisite-patch-id: 2a0eefce67b21d1f30c272fd8115b0dc1aca3897
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-11-12  8:05 [PATCH] perf: Fix 0 count issue of cpu-clock Dapeng Mi
  2025-11-12 16:42 ` Ian Rogers
  2025-11-18  1:43 ` Namhyung Kim
@ 2025-11-18 10:50 ` Peter Zijlstra
  2025-11-18 11:03 ` Peter Zijlstra
  3 siblings, 0 replies; 11+ messages in thread
From: Peter Zijlstra @ 2025-11-18 10:50 UTC (permalink / raw)
  To: Dapeng Mi
  Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers,
	Adrian Hunter, Alexander Shishkin, Andi Kleen, Eranian Stephane,
	linux-kernel, linux-perf-users, Dapeng Mi, Zide Chen,
	Falcon Thomas, Xudong Hao

On Wed, Nov 12, 2025 at 04:05:26PM +0800, Dapeng Mi wrote:
> Currently cpu-clock event always returns 0 count, e.g.,
> 
> perf stat -e cpu-clock -- sleep 1
> 
>  Performance counter stats for 'sleep 1':
>                  0      cpu-clock                        #    0.000 CPUs utilized
>        1.002308394 seconds time elapsed
> 
> The root cause is the commit 'bc4394e5e79c ("perf: Fix the throttle
>  error of some clock events")' adds PERF_EF_UPDATE flag check before
> calling cpu_clock_event_update() to update the count, however the
> PERF_EF_UPDATE flag is never set when the cpu-clock event is stopped in
> counting mode (pmu->dev() -> cpu_clock_event_del() ->
> cpu_clock_event_stop()). This leads to the cpu-clock event count is
> never updated.
> 
> To fix this issue, force to set PERF_EF_UPDATE flag for cpu-clock event
> just like what task-clock does. Besides, or flags with PERF_EF_UPDATE
> for task-clock although currently the flags argument would always be 0.
> 
> Fixes: bc4394e5e79c ("perf: Fix the throttle error of some clock events")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---

So I'm forever struggling to find what tree a particular commit is in,
but afaict the above fingered commit is already in Linus' tree and so
this should go to perf/urgent, right?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-11-12  8:05 [PATCH] perf: Fix 0 count issue of cpu-clock Dapeng Mi
                   ` (2 preceding siblings ...)
  2025-11-18 10:50 ` Peter Zijlstra
@ 2025-11-18 11:03 ` Peter Zijlstra
  2025-11-18 11:04   ` Peter Zijlstra
  3 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2025-11-18 11:03 UTC (permalink / raw)
  To: Dapeng Mi
  Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers,
	Adrian Hunter, Alexander Shishkin, Andi Kleen, Eranian Stephane,
	linux-kernel, linux-perf-users, Dapeng Mi, Zide Chen,
	Falcon Thomas, Xudong Hao

On Wed, Nov 12, 2025 at 04:05:26PM +0800, Dapeng Mi wrote:

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f6a08c73f783..77d3af5959c1 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11964,7 +11964,7 @@ static int cpu_clock_event_add(struct perf_event *event, int flags)
>  
>  static void cpu_clock_event_del(struct perf_event *event, int flags)
>  {
> -	cpu_clock_event_stop(event, flags);
> +	cpu_clock_event_stop(event, flags | PERF_EF_UPDATE);
>  }
>  
>  static void cpu_clock_event_read(struct perf_event *event)
> @@ -12043,7 +12043,7 @@ static int task_clock_event_add(struct perf_event *event, int flags)
>  
>  static void task_clock_event_del(struct perf_event *event, int flags)
>  {
> -	task_clock_event_stop(event, PERF_EF_UPDATE);
> +	task_clock_event_stop(event, flags | PERF_EF_UPDATE);
>  }

I think it can both just be: PERF_EF_UPDATE. The only pmu::del() caller
hands in flags=0, but if there were to be flags added, we'd have to
audit all del methods anyway.

Also, the few comments we do have already note that ->del() must do
->stop(EF_UPDATE).

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-11-18 11:03 ` Peter Zijlstra
@ 2025-11-18 11:04   ` Peter Zijlstra
  2025-11-18 11:22     ` Mi, Dapeng
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2025-11-18 11:04 UTC (permalink / raw)
  To: Dapeng Mi
  Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers,
	Adrian Hunter, Alexander Shishkin, Andi Kleen, Eranian Stephane,
	linux-kernel, linux-perf-users, Dapeng Mi, Zide Chen,
	Falcon Thomas, Xudong Hao

On Tue, Nov 18, 2025 at 12:03:09PM +0100, Peter Zijlstra wrote:
> On Wed, Nov 12, 2025 at 04:05:26PM +0800, Dapeng Mi wrote:
> 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index f6a08c73f783..77d3af5959c1 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -11964,7 +11964,7 @@ static int cpu_clock_event_add(struct perf_event *event, int flags)
> >  
> >  static void cpu_clock_event_del(struct perf_event *event, int flags)
> >  {
> > -	cpu_clock_event_stop(event, flags);
> > +	cpu_clock_event_stop(event, flags | PERF_EF_UPDATE);
> >  }
> >  
> >  static void cpu_clock_event_read(struct perf_event *event)
> > @@ -12043,7 +12043,7 @@ static int task_clock_event_add(struct perf_event *event, int flags)
> >  
> >  static void task_clock_event_del(struct perf_event *event, int flags)
> >  {
> > -	task_clock_event_stop(event, PERF_EF_UPDATE);
> > +	task_clock_event_stop(event, flags | PERF_EF_UPDATE);
> >  }
> 
> I think it can both just be: PERF_EF_UPDATE. The only pmu::del() caller
> hands in flags=0, but if there were to be flags added, we'd have to
> audit all del methods anyway.
> 
> Also, the few comments we do have already note that ->del() must do
> ->stop(EF_UPDATE).

Updated patch now sits in queue/perf/urgent.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-11-18 11:04   ` Peter Zijlstra
@ 2025-11-18 11:22     ` Mi, Dapeng
  2025-11-18 11:24       ` Peter Zijlstra
  0 siblings, 1 reply; 11+ messages in thread
From: Mi, Dapeng @ 2025-11-18 11:22 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers,
	Adrian Hunter, Alexander Shishkin, Andi Kleen, Eranian Stephane,
	linux-kernel, linux-perf-users, Dapeng Mi, Zide Chen,
	Falcon Thomas, Xudong Hao


On 11/18/2025 7:04 PM, Peter Zijlstra wrote:
> On Tue, Nov 18, 2025 at 12:03:09PM +0100, Peter Zijlstra wrote:
>> On Wed, Nov 12, 2025 at 04:05:26PM +0800, Dapeng Mi wrote:
>>
>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>> index f6a08c73f783..77d3af5959c1 100644
>>> --- a/kernel/events/core.c
>>> +++ b/kernel/events/core.c
>>> @@ -11964,7 +11964,7 @@ static int cpu_clock_event_add(struct perf_event *event, int flags)
>>>  
>>>  static void cpu_clock_event_del(struct perf_event *event, int flags)
>>>  {
>>> -	cpu_clock_event_stop(event, flags);
>>> +	cpu_clock_event_stop(event, flags | PERF_EF_UPDATE);
>>>  }
>>>  
>>>  static void cpu_clock_event_read(struct perf_event *event)
>>> @@ -12043,7 +12043,7 @@ static int task_clock_event_add(struct perf_event *event, int flags)
>>>  
>>>  static void task_clock_event_del(struct perf_event *event, int flags)
>>>  {
>>> -	task_clock_event_stop(event, PERF_EF_UPDATE);
>>> +	task_clock_event_stop(event, flags | PERF_EF_UPDATE);
>>>  }
>> I think it can both just be: PERF_EF_UPDATE. The only pmu::del() caller
>> hands in flags=0, but if there were to be flags added, we'd have to
>> audit all del methods anyway.
>>
>> Also, the few comments we do have already note that ->del() must do
>> ->stop(EF_UPDATE).
> Updated patch now sits in queue/perf/urgent.

Hi Peter,

Thanks for merging. If we decide not to or the "flags", then the last
sentence "Besides, or flags with PERF_EF_UPDATE for task-clock although
currently the flags argument would always be 0." in the commit message
should be dropped as well. :)

https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=perf/urgent&id=cfaecad27435b4eb6a22bb9d008fec4984e03a21

Thanks,

Dapeng Mi


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-11-18 11:22     ` Mi, Dapeng
@ 2025-11-18 11:24       ` Peter Zijlstra
  2025-12-05 23:44         ` Ian Rogers
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2025-11-18 11:24 UTC (permalink / raw)
  To: Mi, Dapeng
  Cc: Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Ian Rogers,
	Adrian Hunter, Alexander Shishkin, Andi Kleen, Eranian Stephane,
	linux-kernel, linux-perf-users, Dapeng Mi, Zide Chen,
	Falcon Thomas, Xudong Hao

On Tue, Nov 18, 2025 at 07:22:24PM +0800, Mi, Dapeng wrote:
> 
> On 11/18/2025 7:04 PM, Peter Zijlstra wrote:
> > On Tue, Nov 18, 2025 at 12:03:09PM +0100, Peter Zijlstra wrote:
> >> On Wed, Nov 12, 2025 at 04:05:26PM +0800, Dapeng Mi wrote:
> >>
> >>> diff --git a/kernel/events/core.c b/kernel/events/core.c
> >>> index f6a08c73f783..77d3af5959c1 100644
> >>> --- a/kernel/events/core.c
> >>> +++ b/kernel/events/core.c
> >>> @@ -11964,7 +11964,7 @@ static int cpu_clock_event_add(struct perf_event *event, int flags)
> >>>  
> >>>  static void cpu_clock_event_del(struct perf_event *event, int flags)
> >>>  {
> >>> -	cpu_clock_event_stop(event, flags);
> >>> +	cpu_clock_event_stop(event, flags | PERF_EF_UPDATE);
> >>>  }
> >>>  
> >>>  static void cpu_clock_event_read(struct perf_event *event)
> >>> @@ -12043,7 +12043,7 @@ static int task_clock_event_add(struct perf_event *event, int flags)
> >>>  
> >>>  static void task_clock_event_del(struct perf_event *event, int flags)
> >>>  {
> >>> -	task_clock_event_stop(event, PERF_EF_UPDATE);
> >>> +	task_clock_event_stop(event, flags | PERF_EF_UPDATE);
> >>>  }
> >> I think it can both just be: PERF_EF_UPDATE. The only pmu::del() caller
> >> hands in flags=0, but if there were to be flags added, we'd have to
> >> audit all del methods anyway.
> >>
> >> Also, the few comments we do have already note that ->del() must do
> >> ->stop(EF_UPDATE).
> > Updated patch now sits in queue/perf/urgent.
> 
> Hi Peter,
> 
> Thanks for merging. If we decide not to or the "flags", then the last
> sentence "Besides, or flags with PERF_EF_UPDATE for task-clock although
> currently the flags argument would always be 0." in the commit message
> should be dropped as well. :)

Should be fixed now. Thanks!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-11-18 11:24       ` Peter Zijlstra
@ 2025-12-05 23:44         ` Ian Rogers
  2025-12-08  5:16           ` Mi, Dapeng
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Rogers @ 2025-12-05 23:44 UTC (permalink / raw)
  To: Peter Zijlstra, Mi, Dapeng, Ingo Molnar
  Cc: Arnaldo Carvalho de Melo, Namhyung Kim, Adrian Hunter,
	Alexander Shishkin, Andi Kleen, Eranian Stephane, linux-kernel,
	linux-perf-users, Dapeng Mi, Zide Chen, Falcon Thomas, Xudong Hao

On Tue, Nov 18, 2025 at 3:24 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Nov 18, 2025 at 07:22:24PM +0800, Mi, Dapeng wrote:
> >
> > On 11/18/2025 7:04 PM, Peter Zijlstra wrote:
> > > On Tue, Nov 18, 2025 at 12:03:09PM +0100, Peter Zijlstra wrote:
> > >> Also, the few comments we do have already note that ->del() must do
> > >> ->stop(EF_UPDATE).

Hi Peter,

I was trying to do a quick sanity check that other callers of stop
were passing in EF_UPDATE. I wondered if the case of
perf_event_throttle_group with leader sampling has an issue? The
throttle should only happen on an event or group leader that is in
sampling mode, but the group members could be non-sampling with leader
sampling. Sorry for the noise if I'm wrong on this. Fwiw, the other
uses either pass in EF_UPDATE or I think for  __perf_event_aux_pause
and  __perf_event_overflow the problem isn't relevant.

Thanks,
Ian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] perf: Fix 0 count issue of cpu-clock
  2025-12-05 23:44         ` Ian Rogers
@ 2025-12-08  5:16           ` Mi, Dapeng
  0 siblings, 0 replies; 11+ messages in thread
From: Mi, Dapeng @ 2025-12-08  5:16 UTC (permalink / raw)
  To: Ian Rogers, Peter Zijlstra, Ingo Molnar
  Cc: Arnaldo Carvalho de Melo, Namhyung Kim, Adrian Hunter,
	Alexander Shishkin, Andi Kleen, Eranian Stephane, linux-kernel,
	linux-perf-users, Dapeng Mi, Zide Chen, Falcon Thomas, Xudong Hao


On 12/6/2025 7:44 AM, Ian Rogers wrote:
> On Tue, Nov 18, 2025 at 3:24 AM Peter Zijlstra <peterz@infradead.org> wrote:
>> On Tue, Nov 18, 2025 at 07:22:24PM +0800, Mi, Dapeng wrote:
>>> On 11/18/2025 7:04 PM, Peter Zijlstra wrote:
>>>> On Tue, Nov 18, 2025 at 12:03:09PM +0100, Peter Zijlstra wrote:
>>>>> Also, the few comments we do have already note that ->del() must do
>>>>> ->stop(EF_UPDATE).
> Hi Peter,
>
> I was trying to do a quick sanity check that other callers of stop
> were passing in EF_UPDATE. I wondered if the case of
> perf_event_throttle_group with leader sampling has an issue? The
> throttle should only happen on an event or group leader that is in
> sampling mode, but the group members could be non-sampling with leader
> sampling. Sorry for the noise if I'm wrong on this. Fwiw, the other
> uses either pass in EF_UPDATE or I think for  __perf_event_aux_pause
> and  __perf_event_overflow the problem isn't relevant.

Base on the logic, it looks we need to add the flag EF_UPDATE in
perf_event_throttle_group(), but need Peter's confirm. 


>
> Thanks,
> Ian
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-12-08  5:17 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-12  8:05 [PATCH] perf: Fix 0 count issue of cpu-clock Dapeng Mi
2025-11-12 16:42 ` Ian Rogers
2025-11-17 17:04   ` Ian Rogers
2025-11-18  1:43 ` Namhyung Kim
2025-11-18 10:50 ` Peter Zijlstra
2025-11-18 11:03 ` Peter Zijlstra
2025-11-18 11:04   ` Peter Zijlstra
2025-11-18 11:22     ` Mi, Dapeng
2025-11-18 11:24       ` Peter Zijlstra
2025-12-05 23:44         ` Ian Rogers
2025-12-08  5:16           ` Mi, Dapeng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).