[lkml]   [2010]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
SubjectRe: [PATCH 0/3] Generalise ARM perf-events backend for oprofile
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

> 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);
+ }
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) {}

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



 \ /
  Last update: 2010-08-23 17:59    [W:0.068 / U:2.644 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site