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

[Ravi Bangoria <ravi.bangoria@amd.com>]

>>> 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.

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).

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:

----><----

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