Re: [PATCH v4 1/4] perf/core: Rework forwarding of {task|cpu}-clock events

From: Pengfei Xu <pengfei.xu@intel.com>
To: Ravi Bangoria <ravi.bangoria@amd.com>
Cc: "peterz@infradead.org" <peterz@infradead.org>,
	"namhyung@kernel.org" <namhyung@kernel.org>,
	"Eranian, Stephane" <eranian@google.com>,
	"acme@kernel.org" <acme@kernel.org>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"jolsa@kernel.org" <jolsa@kernel.org>,
	"irogers@google.com" <irogers@google.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"kan.liang@linux.intel.com" <kan.liang@linux.intel.com>,
	"Hunter, Adrian" <adrian.hunter@intel.com>,
	"maddy@linux.ibm.com" <maddy@linux.ibm.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"linux-perf-users@vger.kernel.org"
	<linux-perf-users@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"sandipan.das@amd.com" <sandipan.das@amd.com>,
	"ananth.narayan@amd.com" <ananth.narayan@amd.com>,
	"santosh.shukla@amd.com" <santosh.shukla@amd.com>,
	lkp <lkp@intel.com>
Subject: Re: [PATCH v4 1/4] perf/core: Rework forwarding of {task|cpu}-clock events
Date: Thu, 29 Feb 2024 11:41:29 +0800	[thread overview]
Message-ID: <Zd/86VJB/EQZsYPD@xpf.sh.intel.com> (raw)
In-Reply-To: <00f32f45-a423-2c6a-e53b-c132ed1cde50@amd.com>

On 2024-02-28 at 20:49:59 +0800, Ravi Bangoria wrote:
> >>> Currently, PERF_TYPE_SOFTWARE is treated specially since task-clock and
> >>> cpu-clock events are interfaced through it but internally gets forwarded
> >>> to their own pmus.
> >>>
> >>> Rework this by overwriting event->attr.type in perf_swevent_init() which
> >>> will cause perf_init_event() to retry with updated type and event will
> >>> automatically get forwarded to right pmu. With the change, SW pmu no
> >>> longer needs to be treated specially and can be included in 'pmu_idr'
> >>> list.
> >>>
> >>> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> >>> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> >>> ---
> >>>  include/linux/perf_event.h | 10 +++++
> >>>  kernel/events/core.c       | 77 ++++++++++++++++++++------------------
> >>>  2 files changed, 51 insertions(+), 36 deletions(-)
> >>
> >> Greeting!
> >> There is task hung in perf_tp_event_init in v6.8-rc4 in guest.
> > 
> > Thanks for the bug report. I'm able to reproduce it. Will try to spend
> > more time to rootcause it.
> 
> Although the bisect has lead to 0d6d062ca27e as culprit commit, a minor
> change (shown below) in the test program can create the same task hang
> issue even with 0d6d062ca27e reverted.
> 
> -   *(uint32_t*)0x200000c0 = 6;   /* Use cpu-clock pmu type when 0d6d062ca27e is present */
> +   *(uint32_t*)0x200000c0 = 1;   /* Use software pmu type when 0d6d062ca27e is absent */
> 
> So, 0d6d062ca27e is not the culprit commit.

Thank you for your analysis and additional verification!

commit 0d6d062ca27e only changes kernel/events/core.c judgement:
PERF_TYPE_SOFTWARE to perf_cpu_clock.type in function cpu_clock_event_init():
"
@@ -11086,7 +11092,7 @@ static void cpu_clock_event_read(struct perf_event *event)
 
  static int cpu_clock_event_init(struct perf_event *event)
   {
   -       if (event->attr.type != PERF_TYPE_SOFTWARE)
   +       if (event->attr.type != perf_cpu_clock.type)
                   return -ENOENT;
"

After reverted the commit, and change test code 0x200000c0 from 6 to 1,
kernel code in kernel/events/core.c: func perf_swevent_init() still keeps
PERF_TYPE_SOFTWARE(1) judgement because above commit doesn't change judgement
in perf_swevent_init():
"
// PERF_TYPE_SOFTWARE = 1;
	if (event->attr.type != PERF_TYPE_SOFTWARE)
			return -ENOENT;
"

So it seems show that attr.type(0x200000c0) = 6 will return -2 and not solve
further action in perf_swevent_init() will not trigger task hang.

And if attr.type(0x200000c0) = 1 will pass the judgement and solve further
action in perf_swevent_init(), and then will trigger task hang.

Thanks for correction if there is something wrong.


> 
> Additionally,
> 
> o I've seen task hang or soft-lockups on a single cpu KVM guest while
>   running your test as root and also as normal user with
>   perf_event_paranoid=-1. But the same experiment on host, no lockups,
>   only task hang. So I feel the bug report is false positive and there
>   is no real issue (since the experiment requires special privilege).

Thanks for your info! Seems some problem will cause cpu soft-lockups.
If there is more clue, will update it.

> 
> o 0d6d062ca27e has inadvertently started allowing cpu-clock and task-
>   clock events creation via their own pmu->type in perf_event_open(),
>   instead of earlier design where the only interface was through sw
>   pmu. Is it harmful? Probably not. But worth to be documented:

Thanks a lot for description, and there is some other way to trigger
the perf_event type change.

Thanks!

> 
> ----><----
> 
> From c7ae1c57e2a23a05eb982524d37bc8542c9c9a34 Mon Sep 17 00:00:00 2001
> From: Ravi Bangoria <ravi.bangoria@amd.com>
> Date: Wed, 28 Feb 2024 17:29:04 +0530
> Subject: [PATCH] perf/core: Document {cpu|task}-clock event open behavior
> 
> The standard interface to invoke task-clock and cpu-clock pmu is through
> software pmu (see perf_swevent_init()), since these pmus are not exposed
> to the user via sysfs and thus user doesn't know their pmu->type. However,
> current code allows user to open an event if user has passed correct type
> in the perf event attribute. This is not easily apparent from the code and
> thus worth to be documented.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
>  kernel/events/core.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f0f0f71213a1..4072bccab3ba 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -11178,6 +11178,13 @@ static void cpu_clock_event_read(struct perf_event *event)
>  
>  static int cpu_clock_event_init(struct perf_event *event)
>  {
> +	/*
> +	 * The standard interface to invoke task-clock pmu is through software
> +	 * pmu(see perf_swevent_init()), since task-clock pmu is not exposed to
> +	 * the user via sysfs and thus user doesn't know perf_task_clock.type.
> +	 * However, allow user to open an event if user has passed correct type
> +	 * in the attribute.
> +	 */
>  	if (event->attr.type != perf_cpu_clock.type)
>  		return -ENOENT;
>  
> @@ -11260,6 +11267,13 @@ static void task_clock_event_read(struct perf_event *event)
>  
>  static int task_clock_event_init(struct perf_event *event)
>  {
> +	/*
> +	 * The standard interface to invoke task-clock pmu is through software
> +	 * pmu(see perf_swevent_init()), since task-clock pmu is not exposed to
> +	 * the user via sysfs and thus user doesn't know perf_task_clock.type.
> +	 * However, allow user to open an event if user has passed correct type
> +	 * in the attribute.
> +	 */
>  	if (event->attr.type != perf_task_clock.type)
>  		return -ENOENT;
>  
> -- 
> 2.34.1
