Re: [PATCH 0/3] Generalise ARM perf-events backend for oprofile

From: Will Deacon
Date: Mon Aug 23 2010 - 11:56:07 EST


Hi Matt,

On Mon, 2010-08-23 at 11:46 +0100, Matt Fleming wrote:
> The perf-events backend for OProfile that Will Deacon wrote in
> 8c1fc96f6fd1f361428ba805103af0d0eee65179 ("ARM: 6072/1: oprofile: use
> perf-events framework as backend") is of use to more architectures
> than just ARM. Move the code into drivers/oprofile/ so that SH can use
> it instead of the nearly identical copy of its OProfile code.
>
> The benefit of the backend is that it becomes necessary to only
> maintain one copy of the PMU accessor functions for each architecture,
> with bug fixes and new features benefiting both OProfile and perf.
>
The downside is that it's only really applicable if all the subarch
targets which have OProfile support have equivalent perf support. I know
this is the case for SH and ARM, but I'm not sure about other
architectures.

> Note that I haven't been able to test these patches on an ARM board to
> see if I've caused any regressions. If anyone else could do that I'd
> appreciate it.
>
I tried to test them but they don't compile:

arch/arm/oprofile/common.c: In function 'oprofile_arch_exit':
arch/arm/oprofile/common.c:234: error: 'perf_events' undeclared (first use in this function)
arch/arm/oprofile/common.c:234: error: (Each undeclared identifier is reported only once
arch/arm/oprofile/common.c:234: error: for each function it appears in.)
arch/arm/oprofile/common.c:237: error: 'perf_num_counters' undeclared (first use in this function)
arch/arm/oprofile/common.c:246: error: 'counter_config' undeclared (first use in this function)

This is because the oprofile_arch_exit implementation for ARM frees
data structures that were previously allocated in oprofile_arch_init.
Since this is now done in op_perf_create_files, I'm not sure where the
freeing should be done. OProfile can be compiled as a module, so this
does need to be implemented somewhere (plus, if oprofile_arch_init fails
oprofile_arch_exit is called immediately). Perhaps an op_perf_exit()
function could be called from the arch code?

Looking at the existing ARM implementation, it's not entirely safe in
the case that oprofile_arch_init fails and needs something like:

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..15d379f 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -275,10 +275,12 @@ out:
return ret;
}

-static void exit_driverfs(void)
+static void __exit exit_driverfs(void)
{
- platform_device_unregister(oprofile_pdev);
- platform_driver_unregister(&oprofile_driver);
+ if (!IS_ERR_OR_NULL(oprofile_pdev)) {
+ platform_device_unregister(oprofile_pdev);
+ platform_driver_unregister(&oprofile_driver);
+ }
}
#else
static int __init init_driverfs(void) { return 0; }
@@ -363,10 +365,8 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
}

ret = init_driverfs();
- if (ret) {
- kfree(counter_config);
+ if (ret)
return ret;
- }

for_each_possible_cpu(cpu) {
perf_events[cpu] = kcalloc(perf_num_counters,
@@ -396,13 +396,14 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
return ret;
}

-void oprofile_arch_exit(void)
+void __exit oprofile_arch_exit(void)
{
int cpu, id;
struct perf_event *event;

+ exit_driverfs();
+
if (*perf_events) {
- exit_driverfs();
for_each_possible_cpu(cpu) {
for (id = 0; id < perf_num_counters; ++id) {
event = perf_events[cpu][id];
@@ -422,5 +423,5 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
pr_info("oprofile: hardware counters not available\n");
return -ENODEV;
}
-void oprofile_arch_exit(void) {}
+void __exit oprofile_arch_exit(void) {}
#endif /* CONFIG_HW_PERF_EVENTS */


I can submit this as a separate patch or you can fold it into your changes
to avoid any conflicts.

Cheers,

Will

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/