| Message ID | 20240823113306.2310957-2-leo.yan@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf auxtrace: Support multiple AUX events | expand |
Hi Peter, Adrian, On 8/23/2024 12:32 PM, Leo Yan wrote: > > This commit changes the condition from checking the same PMU instance to > checking the same .setup_aux() callback pointer. If PMU events have the > same callback pointer, it means they share the same PMU driver module. > This allows support for multiple PMU events with the same driver module. > > As a result, more than one AUX event (e.g. arm_spe_0 and arm_spe_1) > can record trace into the AUX ring buffer. This patch is the only change in the kernel, so it is crucial for this series. Can I get your opinion? Thanks a lot! Leo > Signed-off-by: Leo Yan <leo.yan@arm.com> > --- > kernel/events/core.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index c973e3c11e03..883c457911a3 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -12345,9 +12345,16 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) > > /* > * If both events generate aux data, they must be on the same PMU > + * module but can be with different PMU instances. > + * > + * For a built-in PMU module, the 'pmu->module' pointer is NULL, > + * thus it is not feasible to compare the module pointers when > + * AUX PMU drivers are built into the kernel image. Instead, > + * comparing the .setup_aux() callback pointer can determine if > + * the two PMU events come from the same PMU driver. > */ > if (has_aux(event) && has_aux(output_event) && > - event->pmu != output_event->pmu) > + event->pmu->setup_aux != output_event->pmu->setup_aux) > goto out; > > /* > -- > 2.34.1 >
On 23/08/24 14:32, Leo Yan wrote: > This commit changes the condition from checking the same PMU instance to > checking the same .setup_aux() callback pointer. If PMU events have the > same callback pointer, it means they share the same PMU driver module. > This allows support for multiple PMU events with the same driver module. > > As a result, more than one AUX event (e.g. arm_spe_0 and arm_spe_1) > can record trace into the AUX ring buffer. > > Signed-off-by: Leo Yan <leo.yan@arm.com> > --- > kernel/events/core.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index c973e3c11e03..883c457911a3 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -12345,9 +12345,16 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) > > /* > * If both events generate aux data, they must be on the same PMU > + * module but can be with different PMU instances. > + * > + * For a built-in PMU module, the 'pmu->module' pointer is NULL, > + * thus it is not feasible to compare the module pointers when > + * AUX PMU drivers are built into the kernel image. Instead, > + * comparing the .setup_aux() callback pointer can determine if > + * the two PMU events come from the same PMU driver. > */ > if (has_aux(event) && has_aux(output_event) && > - event->pmu != output_event->pmu) > + event->pmu->setup_aux != output_event->pmu->setup_aux) It is not very flexible and risks someone adding aux PMUs that do not want that rule but accidentally support it. Another option is to add a PMU callback, but really you need to Peter's feedback.
On 9/3/2024 11:06 AM, Adrian Hunter wrote: >> @@ -12345,9 +12345,16 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) >> >> /* >> * If both events generate aux data, they must be on the same PMU >> + * module but can be with different PMU instances. >> + * >> + * For a built-in PMU module, the 'pmu->module' pointer is NULL, >> + * thus it is not feasible to compare the module pointers when >> + * AUX PMU drivers are built into the kernel image. Instead, >> + * comparing the .setup_aux() callback pointer can determine if >> + * the two PMU events come from the same PMU driver. >> */ >> if (has_aux(event) && has_aux(output_event) && >> - event->pmu != output_event->pmu) >> + event->pmu->setup_aux != output_event->pmu->setup_aux) > > It is not very flexible and risks someone adding aux PMUs that > do not want that rule but accidentally support it. Another > option is to add a PMU callback, but really you need to Peter's > feedback. Thanks a lot for sharing opinion, Adrian! How about below code? An alternative way is to compare the PMU's parent device driver, e.g. for Arm SPE PMU events, this can compare if two PMU events are using the Arm SPE driver. /* * If both events generate aux data, they must be on the same PMU * module but can be with different PMU instances. */ if (has_aux(event) && has_aux(output_event)) { /* It isn't allowed if it fails to find driver pointer */ if (!event->pmu->parent || !event->pmu->parent->driver) goto out; if (!output_event->pmu->parent || !output_event->pmu->parent->driver) goto out; /* * It isn't allowed if aux events are not same type of PMU * device. This is determined by comparing the associated * driver pointers. */ if (event->pmu->parent->driver != output_event->pmu->parent->driver) goto out; } I verified the code above, it also works well at my side. @Peter.Z, Please let me know if this is okay for you. Thanks, Leo
On 4/09/24 22:35, Leo Yan wrote: > On 9/3/2024 11:06 AM, Adrian Hunter wrote: >>> @@ -12345,9 +12345,16 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) >>> >>> /* >>> * If both events generate aux data, they must be on the same PMU >>> + * module but can be with different PMU instances. >>> + * >>> + * For a built-in PMU module, the 'pmu->module' pointer is NULL, >>> + * thus it is not feasible to compare the module pointers when >>> + * AUX PMU drivers are built into the kernel image. Instead, >>> + * comparing the .setup_aux() callback pointer can determine if >>> + * the two PMU events come from the same PMU driver. >>> */ >>> if (has_aux(event) && has_aux(output_event) && >>> - event->pmu != output_event->pmu) >>> + event->pmu->setup_aux != output_event->pmu->setup_aux) >> >> It is not very flexible and risks someone adding aux PMUs that >> do not want that rule but accidentally support it. Another >> option is to add a PMU callback, but really you need to Peter's >> feedback. > > Thanks a lot for sharing opinion, Adrian! > > How about below code? An alternative way is to compare the PMU's parent > device driver, e.g. for Arm SPE PMU events, this can compare if two PMU > events are using the Arm SPE driver. IMHO, in the general case, whether 2 AUX area events can output to the same buffer isn't really related to the device hierarchy, driver or module. > > /* > * If both events generate aux data, they must be on the same PMU > * module but can be with different PMU instances. > */ > if (has_aux(event) && has_aux(output_event)) { > /* It isn't allowed if it fails to find driver pointer */ > if (!event->pmu->parent || !event->pmu->parent->driver) > goto out; > > if (!output_event->pmu->parent || !output_event->pmu->parent->driver) > goto out; > > /* > * It isn't allowed if aux events are not same type of PMU > * device. This is determined by comparing the associated > * driver pointers. > */ > if (event->pmu->parent->driver != output_event->pmu->parent->driver) > goto out; > } > > I verified the code above, it also works well at my side. > > @Peter.Z, Please let me know if this is okay for you. > > Thanks, > Leo
diff --git a/kernel/events/core.c b/kernel/events/core.c index c973e3c11e03..883c457911a3 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -12345,9 +12345,16 @@ perf_event_set_output(struct perf_event *event, struct perf_event *output_event) /* * If both events generate aux data, they must be on the same PMU + * module but can be with different PMU instances. + * + * For a built-in PMU module, the 'pmu->module' pointer is NULL, + * thus it is not feasible to compare the module pointers when + * AUX PMU drivers are built into the kernel image. Instead, + * comparing the .setup_aux() callback pointer can determine if + * the two PMU events come from the same PMU driver. */ if (has_aux(event) && has_aux(output_event) && - event->pmu != output_event->pmu) + event->pmu->setup_aux != output_event->pmu->setup_aux) goto out; /*
This commit changes the condition from checking the same PMU instance to checking the same .setup_aux() callback pointer. If PMU events have the same callback pointer, it means they share the same PMU driver module. This allows support for multiple PMU events with the same driver module. As a result, more than one AUX event (e.g. arm_spe_0 and arm_spe_1) can record trace into the AUX ring buffer. Signed-off-by: Leo Yan <leo.yan@arm.com> --- kernel/events/core.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)