Messages in this thread Patch in this message | | | Date | Thu, 29 Feb 2024 11:41:29 +0800 | From | Pengfei Xu <> | Subject | Re: [PATCH v4 1/4] perf/core: Rework forwarding of {task|cpu}-clock events |
| |
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
| |