[PATCH V2 0/4] Generalise ARM perf-events backend for oprofile

[PATCH V2 0/4] Generalise ARM perf-events backend for oprofile

[Matt Fleming]

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.

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. Though, I have been able to compile this version of the
series.

This patch series is based on tip/master.

Changes from v1:
 - Prefix the new functons with "oprofile_" instead of "op_".
 - Fix ARM compilation errors
 - Move all the oprofile-perf logic into oprofile_perf.c
 - Include cleanup patch from Will

Matt Fleming (3):
  sh: Accessor functions for the sh_pmu state
  oprofile: Abstract the perf-events backend
  sh: Use the perf-events backend for oprofile

Will Deacon (1):
  oprofile: Handle initialisation failure more gracefully

 arch/arm/oprofile/Makefile       |    4 +
 arch/arm/oprofile/common.c       |  228 ++++----------------------------------
 arch/sh/include/asm/perf_event.h |    2 +
 arch/sh/kernel/perf_event.c      |   13 ++
 arch/sh/oprofile/Makefile        |    2 +-
 arch/sh/oprofile/common.c        |   96 +++-------------
 arch/sh/oprofile/op_impl.h       |   33 ------
 drivers/oprofile/oprofile_perf.c |  209 ++++++++++++++++++++++++++++++++++
 include/linux/oprofile.h         |   12 ++
 9 files changed, 283 insertions(+), 316 deletions(-)
 delete mode 100644 arch/sh/oprofile/op_impl.h
 create mode 100644 drivers/oprofile/oprofile_perf.c

[PATCH 1/4] oprofile: Handle initialisation failure more gracefully

[Matt Fleming]

From: Will Deacon <will.deacon@arm.com>

The current implementation is not entirely safe in the case that
oprofile_arch_init() fails. We need to make sure that we always call
exit_driverfs() if we've called init_driverfs(). Also, avoid a potential
double free when freeing 'counter_config', e.g. don't free
'counter_config' in both oprofile_arch_init() and oprofile_arch_exit().

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/oprofile/common.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..482779c 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_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,
@@ -401,8 +401,9 @@ void 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];
-- 
1.7.1

[PATCH 1/4] oprofile: Handle initialisation failure more gracefully

[Robert Richter]

On 26.08.10 15:09:16, Matt Fleming wrote:
> From: Will Deacon <will.deacon@arm.com>
> 
> The current implementation is not entirely safe in the case that
> oprofile_arch_init() fails. We need to make sure that we always call
> exit_driverfs() if we've called init_driverfs(). Also, avoid a potential
> double free when freeing 'counter_config', e.g. don't free
> 'counter_config' in both oprofile_arch_init() and oprofile_arch_exit().
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/oprofile/common.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 0691176..482779c 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_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);
> +	}

The root cause that makes this check necessary is that
oprofile_arch_exit() is called though oprofile_arch_init() failed. We
should better fix this instead. I have to admit we will then have to
check all architectural implementations.

>  }
>  #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);

We should not return from oprofile_arch_init() with allocated
resources if the function fails. To fix duplicate kfrees, we should
free it here and then set counter_config to NULL. It should also be
freed if for_each_possible_cpu() or op_name_from_perf_id() fails.

Also, the pointer should be NULLed after freeing in
oprofile_arch_exit(). There, we also don't need the NULL pointer check
as it is save to call kfree(NULL).

-Robert

> +	if (ret)
>  		return ret;
> -	}
>  
>  	for_each_possible_cpu(cpu) {
>  		perf_events[cpu] = kcalloc(perf_num_counters,
> @@ -401,8 +401,9 @@ void 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];
> -- 
> 1.7.1
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 1/4] oprofile: Handle initialisation failure more gracefully

[Will Deacon]

Hi Robert,

On Fri, 2010-08-27 at 13:43 +0100, Robert Richter wrote:
> On 26.08.10 15:09:16, Matt Fleming wrote:
> > From: Will Deacon <will.deacon@arm.com>
> >
> > The current implementation is not entirely safe in the case that
> > oprofile_arch_init() fails. We need to make sure that we always call
> > exit_driverfs() if we've called init_driverfs(). Also, avoid a potential
> > double free when freeing 'counter_config', e.g. don't free
> > 'counter_config' in both oprofile_arch_init() and oprofile_arch_exit().
> >
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm/oprofile/common.c |   15 ++++++++-------
> >  1 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> > index 0691176..482779c 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_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);
> > +     }
> 
> The root cause that makes this check necessary is that
> oprofile_arch_exit() is called though oprofile_arch_init() failed. We
> should better fix this instead. I have to admit we will then have to
> check all architectural implementations.
> 

I took a look through all of the oprofile_arch_{init,exit} functions
and it looks like only ARM needs fixing. Nobody else does any allocation
here [well, until Matt's unified version comes into play].

> >  }
> >  #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);
> 
> We should not return from oprofile_arch_init() with allocated
> resources if the function fails. To fix duplicate kfrees, we should
> free it here and then set counter_config to NULL. It should also be
> freed if for_each_possible_cpu() or op_name_from_perf_id() fails.
> 
> Also, the pointer should be NULLed after freeing in
> oprofile_arch_exit(). There, we also don't need the NULL pointer check
> as it is save to call kfree(NULL).


How about something like this? This removes the exit call
from the init code and sets pointers to NULL after they have been
freed. We still have to do some checking so that we don't try to
release a NULL perf event:

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..12253eb 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; }
@@ -365,6 +367,7 @@ int __init oprofile_arch_init(struct
oprofile_operations *ops)
 	ret = init_driverfs();
 	if (ret) {
 		kfree(counter_config);
+		counter_config = NULL;
 		return ret;
 	}
 
@@ -374,8 +377,10 @@ int __init oprofile_arch_init(struct
oprofile_operations *ops)
 		if (!perf_events[cpu]) {
 			pr_info("oprofile: failed to allocate %d perf events "
 					"for cpu %d\n", perf_num_counters, cpu);
-			while (--cpu >= 0)
+			while (--cpu >= 0) {
 				kfree(perf_events[cpu]);
+				perf_events[cpu] = NULL;
+			}
 			return -ENOMEM;
 		}
 	}
@@ -396,25 +401,27 @@ 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;
 
-	if (*perf_events) {
-		exit_driverfs();
-		for_each_possible_cpu(cpu) {
-			for (id = 0; id < perf_num_counters; ++id) {
-				event = perf_events[cpu][id];
-				if (event != NULL)
-					perf_event_release_kernel(event);
-			}
-			kfree(perf_events[cpu]);
+	exit_driverfs();
+
+	for_each_possible_cpu(cpu) {
+		if (!perf_events[cpu])
+			continue;
+
+		for (id = 0; id < perf_num_counters; ++id) {
+			event = perf_events[cpu][id];
+			if (event != NULL)
+				perf_event_release_kernel(event);
 		}
+
+		kfree(perf_events[cpu]);
 	}
 
-	if (counter_config)
-		kfree(counter_config);
+	kfree(counter_config);
 }
 #else
 int __init oprofile_arch_init(struct oprofile_operations *ops)
@@ -422,5 +429,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 */
diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
index b336cd9..3094af0 100644
--- a/drivers/oprofile/oprof.c
+++ b/drivers/oprofile/oprof.c
@@ -257,15 +257,11 @@ static int __init oprofile_init(void)
 		printk(KERN_INFO "oprofile: using timer interrupt.\n");
 		err = oprofile_timer_init(&oprofile_ops);
 		if (err)
-			goto out_arch;
+			goto out;
 	}
 	err = oprofilefs_register();
-	if (err)
-		goto out_arch;
-	return 0;
 
-out_arch:
-	oprofile_arch_exit();
+out:
 	return err;
 }
 

Thanks,

Will

[PATCH 1/4] oprofile: Handle initialisation failure more gracefully

[Robert Richter]

On 27.08.10 11:15:25, Will Deacon wrote:

> > > The current implementation is not entirely safe in the case that
> > > oprofile_arch_init() fails. We need to make sure that we always call
> > > exit_driverfs() if we've called init_driverfs(). Also, avoid a potential
> > > double free when freeing 'counter_config', e.g. don't free
> > > 'counter_config' in both oprofile_arch_init() and oprofile_arch_exit().

I am not sure if it is worth the memory handling code, we could
alternativly implement fixed size arrays with MAX_COUNTERS. This would
eas it a lot. But, this code will become generic, so we can stick with
this implementation.

But, cpu varables should be used, maybe with a later patch.

> > The root cause that makes this check necessary is that
> > oprofile_arch_exit() is called though oprofile_arch_init() failed. We
> > should better fix this instead. I have to admit we will then have to
> > check all architectural implementations.
> > 
> 
> I took a look through all of the oprofile_arch_{init,exit} functions
> and it looks like only ARM needs fixing. Nobody else does any allocation
> here [well, until Matt's unified version comes into play].

Great, many thanks.

> How about something like this? This removes the exit call
> from the init code and sets pointers to NULL after they have been
> freed. We still have to do some checking so that we don't try to
> release a NULL perf event:
> 
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 0691176..12253eb 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)) {

This check is obsolete here as we do not call oprofile_arch_exit() on
failure.

> +		platform_device_unregister(oprofile_pdev);
> +		platform_driver_unregister(&oprofile_driver);
> +	}
>  }
>  #else
>  static int __init init_driverfs(void) { return 0; }
> @@ -365,6 +367,7 @@ int __init oprofile_arch_init(struct
> oprofile_operations *ops)
>  	ret = init_driverfs();
>  	if (ret) {
>  		kfree(counter_config);
> +		counter_config = NULL;

Dito, obsolete now.

>  		return ret;
>  	}
>  
> @@ -374,8 +377,10 @@ int __init oprofile_arch_init(struct
> oprofile_operations *ops)
>  		if (!perf_events[cpu]) {
>  			pr_info("oprofile: failed to allocate %d perf events "
>  					"for cpu %d\n", perf_num_counters, cpu);
> -			while (--cpu >= 0)
> +			while (--cpu >= 0) {
>  				kfree(perf_events[cpu]);
> +				perf_events[cpu] = NULL;
> +			}

counter_config must be freed here.

>  			return -ENOMEM;
>  		}
>  	}
> @@ -396,25 +401,27 @@ 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;
>  
> -	if (*perf_events) {
> -		exit_driverfs();
> -		for_each_possible_cpu(cpu) {
> -			for (id = 0; id < perf_num_counters; ++id) {
> -				event = perf_events[cpu][id];
> -				if (event != NULL)
> -					perf_event_release_kernel(event);
> -			}
> -			kfree(perf_events[cpu]);
> +	exit_driverfs();

If we shutdown all this in reverse order, this should be after the
loop.

> +
> +	for_each_possible_cpu(cpu) {
> +		if (!perf_events[cpu])
> +			continue;

Should be never NULL, the check can be removed.

> +
> +		for (id = 0; id < perf_num_counters; ++id) {
> +			event = perf_events[cpu][id];
> +			if (event != NULL)

	if (event)
		...

> +				perf_event_release_kernel(event);
>  		}
> +
> +		kfree(perf_events[cpu]);
>  	}
>  
> -	if (counter_config)
> -		kfree(counter_config);
> +	kfree(counter_config);
>  }
>  #else
>  int __init oprofile_arch_init(struct oprofile_operations *ops)
> @@ -422,5 +429,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 */
> diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
> index b336cd9..3094af0 100644
> --- a/drivers/oprofile/oprof.c
> +++ b/drivers/oprofile/oprof.c
> @@ -257,15 +257,11 @@ static int __init oprofile_init(void)
>  		printk(KERN_INFO "oprofile: using timer interrupt.\n");
>  		err = oprofile_timer_init(&oprofile_ops);
>  		if (err)
> -			goto out_arch;
> +			goto out;

A 'return err;' here would remove the goto. I would preffer this.

Thanks,

-Robert

>  	}
>  	err = oprofilefs_register();
> -	if (err)
> -		goto out_arch;
> -	return 0;
>  
> -out_arch:
> -	oprofile_arch_exit();
> +out:
>  	return err;
>  }
>  
> 
> Thanks,
> 
> Will
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 1/4] oprofile: Handle initialisation failure more gracefully

[Will Deacon]

Hi Robert,

Thanks for taking the time to look at this.

On Fri, 2010-08-27 at 17:38 +0100, Robert Richter wrote:
> On 27.08.10 11:15:25, Will Deacon wrote:

> > How about something like this? This removes the exit call
> > from the init code and sets pointers to NULL after they have been
> > freed. We still have to do some checking so that we don't try to
> > release a NULL perf event:
> >
> > diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> > index 0691176..12253eb 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)) {
> 
> This check is obsolete here as we do not call oprofile_arch_exit() on
> failure.
> 
Sorry, I was being braindead and thought oprofile_arch_exit might be
called via oprofile_exit. As it happens, this is only called because of
module_exit so we know that initialisation will have completed.

In light of this and your comments, I've simplified the code. Once
people are happy with it, I'll post it as a couple of patches:

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 0691176..c2c4a2e 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -275,7 +275,7 @@ out:
        return ret;
 }
 
-static void  exit_driverfs(void)
+static void __exit exit_driverfs(void)
 {
        platform_device_unregister(oprofile_pdev);
        platform_driver_unregister(&oprofile_driver);
@@ -359,14 +359,13 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
        if (!counter_config) {
                pr_info("oprofile: failed to allocate %d "
                                "counters\n", perf_num_counters);
-               return -ENOMEM;
+               ret = -ENOMEM;
+               goto out;
        }
 
        ret = init_driverfs();
-       if (ret) {
-               kfree(counter_config);
-               return ret;
-       }
+       if (ret)
+               goto out;
 
        for_each_possible_cpu(cpu) {
                perf_events[cpu] = kcalloc(perf_num_counters,
@@ -374,9 +373,8 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
                if (!perf_events[cpu]) {
                        pr_info("oprofile: failed to allocate %d perf events "
                                        "for cpu %d\n", perf_num_counters, cpu);
-                       while (--cpu >= 0)
-                               kfree(perf_events[cpu]);
-                       return -ENOMEM;
+                       ret = -ENOMEM;
+                       goto out;
                }
        }
 
@@ -393,28 +391,33 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
        else
                pr_info("oprofile: using %s\n", ops->cpu_type);
 
+out:
+       if (ret) {
+               kfree(counter_config);
+               for_each_possible_cpu(cpu)
+                       kfree(perf_events[cpu]);
+       }
+
        return ret;
 }
 
-void oprofile_arch_exit(void)
+void __exit oprofile_arch_exit(void)
 {
        int cpu, id;
        struct perf_event *event;
 
-       if (*perf_events) {
-               exit_driverfs();
-               for_each_possible_cpu(cpu) {
-                       for (id = 0; id < perf_num_counters; ++id) {
-                               event = perf_events[cpu][id];
-                               if (event != NULL)
-                                       perf_event_release_kernel(event);
-                       }
-                       kfree(perf_events[cpu]);
+       for_each_possible_cpu(cpu) {
+               for (id = 0; id < perf_num_counters; ++id) {
+                       event = perf_events[cpu][id];
+                       if (event)
+                               perf_event_release_kernel(event);
                }
+
+               kfree(perf_events[cpu]);
        }
 
-       if (counter_config)
-               kfree(counter_config);
+       kfree(counter_config);
+       exit_driverfs();
 }
 #else
 int __init oprofile_arch_init(struct oprofile_operations *ops)
@@ -422,5 +425,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 */
diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
index b336cd9..b4a6857 100644
--- a/drivers/oprofile/oprof.c
+++ b/drivers/oprofile/oprof.c
@@ -257,16 +257,9 @@ static int __init oprofile_init(void)
                printk(KERN_INFO "oprofile: using timer interrupt.\n");
                err = oprofile_timer_init(&oprofile_ops);
                if (err)
-                       goto out_arch;
+                       return err;
        }
-       err = oprofilefs_register();
-       if (err)
-               goto out_arch;
-       return 0;
-
-out_arch:
-       oprofile_arch_exit();
-       return err;
+       return oprofilefs_register();
 }

 
Once again, thanks for looking@this and have a good weekend,

Will

[PATCH 1/4] oprofile: Handle initialisation failure more gracefully

[Robert Richter]

On 27.08.10 14:06:45, Will Deacon wrote:
> Hi Robert,
> 
> Thanks for taking the time to look at this.
> 
> On Fri, 2010-08-27 at 17:38 +0100, Robert Richter wrote:
> > On 27.08.10 11:15:25, Will Deacon wrote:
> 
> > > How about something like this? This removes the exit call
> > > from the init code and sets pointers to NULL after they have been
> > > freed. We still have to do some checking so that we don't try to
> > > release a NULL perf event:
> > >
> > > diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> > > index 0691176..12253eb 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)) {
> > 
> > This check is obsolete here as we do not call oprofile_arch_exit() on
> > failure.
> > 
> Sorry, I was being braindead and thought oprofile_arch_exit might be
> called via oprofile_exit. As it happens, this is only called because of
> module_exit so we know that initialisation will have completed.
> 
> In light of this and your comments, I've simplified the code. Once
> people are happy with it, I'll post it as a couple of patches:

Looks good to me know, the code is a lot easier. Please send me a
final version. I'm going to apply it on Monday.

Thanks Will,

-Robert

> 
> diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
> index 0691176..c2c4a2e 100644
> --- a/arch/arm/oprofile/common.c
> +++ b/arch/arm/oprofile/common.c
> @@ -275,7 +275,7 @@ out:
>         return ret;
>  }
>  
> -static void  exit_driverfs(void)
> +static void __exit exit_driverfs(void)
>  {
>         platform_device_unregister(oprofile_pdev);
>         platform_driver_unregister(&oprofile_driver);
> @@ -359,14 +359,13 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>         if (!counter_config) {
>                 pr_info("oprofile: failed to allocate %d "
>                                 "counters\n", perf_num_counters);
> -               return -ENOMEM;
> +               ret = -ENOMEM;
> +               goto out;
>         }
>  
>         ret = init_driverfs();
> -       if (ret) {
> -               kfree(counter_config);
> -               return ret;
> -       }
> +       if (ret)
> +               goto out;
>  
>         for_each_possible_cpu(cpu) {
>                 perf_events[cpu] = kcalloc(perf_num_counters,
> @@ -374,9 +373,8 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>                 if (!perf_events[cpu]) {
>                         pr_info("oprofile: failed to allocate %d perf events "
>                                         "for cpu %d\n", perf_num_counters, cpu);
> -                       while (--cpu >= 0)
> -                               kfree(perf_events[cpu]);
> -                       return -ENOMEM;
> +                       ret = -ENOMEM;
> +                       goto out;
>                 }
>         }
>  
> @@ -393,28 +391,33 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>         else
>                 pr_info("oprofile: using %s\n", ops->cpu_type);
>  
> +out:
> +       if (ret) {
> +               kfree(counter_config);
> +               for_each_possible_cpu(cpu)
> +                       kfree(perf_events[cpu]);
> +       }
> +
>         return ret;
>  }
>  
> -void oprofile_arch_exit(void)
> +void __exit oprofile_arch_exit(void)
>  {
>         int cpu, id;
>         struct perf_event *event;
>  
> -       if (*perf_events) {
> -               exit_driverfs();
> -               for_each_possible_cpu(cpu) {
> -                       for (id = 0; id < perf_num_counters; ++id) {
> -                               event = perf_events[cpu][id];
> -                               if (event != NULL)
> -                                       perf_event_release_kernel(event);
> -                       }
> -                       kfree(perf_events[cpu]);
> +       for_each_possible_cpu(cpu) {
> +               for (id = 0; id < perf_num_counters; ++id) {
> +                       event = perf_events[cpu][id];
> +                       if (event)
> +                               perf_event_release_kernel(event);
>                 }
> +
> +               kfree(perf_events[cpu]);
>         }
>  
> -       if (counter_config)
> -               kfree(counter_config);
> +       kfree(counter_config);
> +       exit_driverfs();
>  }
>  #else
>  int __init oprofile_arch_init(struct oprofile_operations *ops)
> @@ -422,5 +425,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 */
> diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
> index b336cd9..b4a6857 100644
> --- a/drivers/oprofile/oprof.c
> +++ b/drivers/oprofile/oprof.c
> @@ -257,16 +257,9 @@ static int __init oprofile_init(void)
>                 printk(KERN_INFO "oprofile: using timer interrupt.\n");
>                 err = oprofile_timer_init(&oprofile_ops);
>                 if (err)
> -                       goto out_arch;
> +                       return err;
>         }
> -       err = oprofilefs_register();
> -       if (err)
> -               goto out_arch;
> -       return 0;
> -
> -out_arch:
> -       oprofile_arch_exit();
> -       return err;
> +       return oprofilefs_register();
>  }
> 
>  
> Once again, thanks for looking at this and have a good weekend,
> 
> Will
> 
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 2/4] sh: Accessor functions for the sh_pmu state

[Matt Fleming]

Introduce some accessor functions for getting at the name and number of
counters of the current sh_pmu instance.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/sh/include/asm/perf_event.h |    2 ++
 arch/sh/kernel/perf_event.c      |   13 +++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/sh/include/asm/perf_event.h b/arch/sh/include/asm/perf_event.h
index 3d0c9f3..5b7fa84 100644
--- a/arch/sh/include/asm/perf_event.h
+++ b/arch/sh/include/asm/perf_event.h
@@ -25,6 +25,8 @@ struct sh_pmu {
 extern int register_sh_pmu(struct sh_pmu *);
 extern int reserve_pmc_hardware(void);
 extern void release_pmc_hardware(void);
+extern int sh_pmu_num_events(void);
+extern const char *sh_pmu_name(void);
 
 static inline void set_perf_event_pending(void)
 {
diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
index 7a3dc35..086f788 100644
--- a/arch/sh/kernel/perf_event.c
+++ b/arch/sh/kernel/perf_event.c
@@ -60,6 +60,19 @@ static inline int sh_pmu_initialized(void)
 }
 
 /*
+ * Return the number of events for the current sh_pmu.
+ */
+int sh_pmu_num_events(void)
+{
+	return sh_pmu->num_events;
+}
+
+const char *sh_pmu_name(void)
+{
+	return sh_pmu->name;
+}
+
+/*
  * Release the PMU if this is the last perf_event.
  */
 static void hw_perf_event_destroy(struct perf_event *event)
-- 
1.7.1

[PATCH 2/4] sh: Accessor functions for the sh_pmu state

[Robert Richter]

On 26.08.10 15:09:17, Matt Fleming wrote:
> Introduce some accessor functions for getting at the name and number of
> counters of the current sh_pmu instance.
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
>  arch/sh/include/asm/perf_event.h |    2 ++
>  arch/sh/kernel/perf_event.c      |   13 +++++++++++++
>  2 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/sh/include/asm/perf_event.h b/arch/sh/include/asm/perf_event.h
> index 3d0c9f3..5b7fa84 100644
> --- a/arch/sh/include/asm/perf_event.h
> +++ b/arch/sh/include/asm/perf_event.h
> @@ -25,6 +25,8 @@ struct sh_pmu {
>  extern int register_sh_pmu(struct sh_pmu *);
>  extern int reserve_pmc_hardware(void);
>  extern void release_pmc_hardware(void);
> +extern int sh_pmu_num_events(void);
> +extern const char *sh_pmu_name(void);
>  
>  static inline void set_perf_event_pending(void)
>  {
> diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
> index 7a3dc35..086f788 100644
> --- a/arch/sh/kernel/perf_event.c
> +++ b/arch/sh/kernel/perf_event.c
> @@ -60,6 +60,19 @@ static inline int sh_pmu_initialized(void)
>  }
>  
>  /*
> + * Return the number of events for the current sh_pmu.
> + */
> +int sh_pmu_num_events(void)
> +{
> +	return sh_pmu->num_events;
> +}
> +
> +const char *sh_pmu_name(void)
> +{
> +	return sh_pmu->name;
> +}
> +
> +/*

This probably needs EXPORT_SYMBOLS*(), but not really sure. Have you
compiled oprofile as module?

This accessor functions should be generic for all architectures.

-Robert

>   * Release the PMU if this is the last perf_event.
>   */
>  static void hw_perf_event_destroy(struct perf_event *event)
> -- 
> 1.7.1
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH 2/4] sh: Accessor functions for the sh_pmu state

[Matt Fleming]

On Fri, Aug 27, 2010 at 03:43:01PM +0200, Robert Richter wrote:
> On 26.08.10 15:09:17, Matt Fleming wrote:
> > Introduce some accessor functions for getting at the name and number of
> > counters of the current sh_pmu instance.
> > 
> > Signed-off-by: Matt Fleming <matt@console-pimps.org>
> > ---
> >  arch/sh/include/asm/perf_event.h |    2 ++
> >  arch/sh/kernel/perf_event.c      |   13 +++++++++++++
> >  2 files changed, 15 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/sh/include/asm/perf_event.h b/arch/sh/include/asm/perf_event.h
> > index 3d0c9f3..5b7fa84 100644
> > --- a/arch/sh/include/asm/perf_event.h
> > +++ b/arch/sh/include/asm/perf_event.h
> > @@ -25,6 +25,8 @@ struct sh_pmu {
> >  extern int register_sh_pmu(struct sh_pmu *);
> >  extern int reserve_pmc_hardware(void);
> >  extern void release_pmc_hardware(void);
> > +extern int sh_pmu_num_events(void);
> > +extern const char *sh_pmu_name(void);
> >  
> >  static inline void set_perf_event_pending(void)
> >  {
> > diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
> > index 7a3dc35..086f788 100644
> > --- a/arch/sh/kernel/perf_event.c
> > +++ b/arch/sh/kernel/perf_event.c
> > @@ -60,6 +60,19 @@ static inline int sh_pmu_initialized(void)
> >  }
> >  
> >  /*
> > + * Return the number of events for the current sh_pmu.
> > + */
> > +int sh_pmu_num_events(void)
> > +{
> > +	return sh_pmu->num_events;
> > +}
> > +
> > +const char *sh_pmu_name(void)
> > +{
> > +	return sh_pmu->name;
> > +}
> > +
> > +/*
> 
> This probably needs EXPORT_SYMBOLS*(), but not really sure. Have you
> compiled oprofile as module?

Ah, no, I haven't compiled this as a module. I'll add some
EXPORT_SYMBOL()'s in the next version.

> This accessor functions should be generic for all architectures.

This isn't going to work. ARM uses an integer ID whereas SH uses a
string name. This is specific to an architecture and making it generic
would probably involve some abstraction layer.

[PATCH 2/4] sh: Accessor functions for the sh_pmu state

[Robert Richter]

On 27.08.10 15:17:45, Matt Fleming wrote:

> > > --- a/arch/sh/kernel/perf_event.c
> > > +++ b/arch/sh/kernel/perf_event.c
> > > @@ -60,6 +60,19 @@ static inline int sh_pmu_initialized(void)
> > >  }
> > >  
> > >  /*
> > > + * Return the number of events for the current sh_pmu.
> > > + */
> > > +int sh_pmu_num_events(void)
> > > +{
> > > +	return sh_pmu->num_events;
> > > +}
> > > +
> > > +const char *sh_pmu_name(void)
> > > +{
> > > +	return sh_pmu->name;
> > > +}

> > This accessor functions should be generic for all architectures.
> 
> This isn't going to work. ARM uses an integer ID whereas SH uses a
> string name. This is specific to an architecture and making it generic
> would probably involve some abstraction layer.

Perf should provide the interface to detect the number of counters
(btw. *num_events is wrong) and the name of the pmu. The information
is part of perf and thus the functions accessing it should be part of
perf too, not oprofile.

We also need generic functions, because we want to have a generic
oprofile-perf driver.

I don't see that this is hard to implement. We could add function
stubs returning an error or invalid value using the __weak attribute
and implement it for those architectures where we need it.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH V2 3/4] oprofile: Abstract the perf-events backend

[Matt Fleming]

Move the perf-events backend from arch/arm/oprofile into
drivers/oprofile so that the code can be shared between architectures.

This allows each architecture to maintain only a single copy of the
PMU accessor functions instead of one for both perf and OProfile. It
also becomes possible for other architectures to delete much of their
OProfile code in favour of the common code now available in
drivers/oprofile/oprofile_perf.c.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---
 arch/arm/oprofile/Makefile       |    4 +
 arch/arm/oprofile/common.c       |  215 +++-----------------------------------
 drivers/oprofile/oprofile_perf.c |  209 ++++++++++++++++++++++++++++++++++++
 include/linux/oprofile.h         |   12 ++
 4 files changed, 242 insertions(+), 198 deletions(-)
 create mode 100644 drivers/oprofile/oprofile_perf.c

diff --git a/arch/arm/oprofile/Makefile b/arch/arm/oprofile/Makefile
index e666eaf..038d7af 100644
--- a/arch/arm/oprofile/Makefile
+++ b/arch/arm/oprofile/Makefile
@@ -6,4 +6,8 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
 		oprofilefs.o oprofile_stats.o \
 		timer_int.o )
 
+ifeq ($(CONFIG_HW_PERF_EVENTS), y)
+DRIVER_OBJS += $(addprefix ../../../drivers/oprofile/, oprofile_perf.o)
+endif
+
 oprofile-y				:= $(DRIVER_OBJS) common.o
diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 482779c..f4ea5f8 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -25,136 +25,9 @@
 #include <asm/ptrace.h>
 
 #ifdef CONFIG_HW_PERF_EVENTS
-/*
- * Per performance monitor configuration as set via oprofilefs.
- */
-struct op_counter_config {
-	unsigned long count;
-	unsigned long enabled;
-	unsigned long event;
-	unsigned long unit_mask;
-	unsigned long kernel;
-	unsigned long user;
-	struct perf_event_attr attr;
-};
-
 static int op_arm_enabled;
 static DEFINE_MUTEX(op_arm_mutex);
 
-static struct op_counter_config *counter_config;
-static struct perf_event **perf_events[nr_cpumask_bits];
-static int perf_num_counters;
-
-/*
- * Overflow callback for oprofile.
- */
-static void op_overflow_handler(struct perf_event *event, int unused,
-			struct perf_sample_data *data, struct pt_regs *regs)
-{
-	int id;
-	u32 cpu = smp_processor_id();
-
-	for (id = 0; id < perf_num_counters; ++id)
-		if (perf_events[cpu][id] == event)
-			break;
-
-	if (id != perf_num_counters)
-		oprofile_add_sample(regs, id);
-	else
-		pr_warning("oprofile: ignoring spurious overflow "
-				"on cpu %u\n", cpu);
-}
-
-/*
- * Called by op_arm_setup to create perf attributes to mirror the oprofile
- * settings in counter_config. Attributes are created as `pinned' events and
- * so are permanently scheduled on the PMU.
- */
-static void op_perf_setup(void)
-{
-	int i;
-	u32 size = sizeof(struct perf_event_attr);
-	struct perf_event_attr *attr;
-
-	for (i = 0; i < perf_num_counters; ++i) {
-		attr = &counter_config[i].attr;
-		memset(attr, 0, size);
-		attr->type		= PERF_TYPE_RAW;
-		attr->size		= size;
-		attr->config		= counter_config[i].event;
-		attr->sample_period	= counter_config[i].count;
-		attr->pinned		= 1;
-	}
-}
-
-static int op_create_counter(int cpu, int event)
-{
-	int ret = 0;
-	struct perf_event *pevent;
-
-	if (!counter_config[event].enabled || (perf_events[cpu][event] != NULL))
-		return ret;
-
-	pevent = perf_event_create_kernel_counter(&counter_config[event].attr,
-						  cpu, -1,
-						  op_overflow_handler);
-
-	if (IS_ERR(pevent)) {
-		ret = PTR_ERR(pevent);
-	} else if (pevent->state != PERF_EVENT_STATE_ACTIVE) {
-		pr_warning("oprofile: failed to enable event %d "
-				"on CPU %d\n", event, cpu);
-		ret = -EBUSY;
-	} else {
-		perf_events[cpu][event] = pevent;
-	}
-
-	return ret;
-}
-
-static void op_destroy_counter(int cpu, int event)
-{
-	struct perf_event *pevent = perf_events[cpu][event];
-
-	if (pevent) {
-		perf_event_release_kernel(pevent);
-		perf_events[cpu][event] = NULL;
-	}
-}
-
-/*
- * Called by op_arm_start to create active perf events based on the
- * perviously configured attributes.
- */
-static int op_perf_start(void)
-{
-	int cpu, event, ret = 0;
-
-	for_each_online_cpu(cpu) {
-		for (event = 0; event < perf_num_counters; ++event) {
-			ret = op_create_counter(cpu, event);
-			if (ret)
-				goto out;
-		}
-	}
-
-out:
-	return ret;
-}
-
-/*
- * Called by op_arm_stop@the end of a profiling run.
- */
-static void op_perf_stop(void)
-{
-	int cpu, event;
-
-	for_each_online_cpu(cpu)
-		for (event = 0; event < perf_num_counters; ++event)
-			op_destroy_counter(cpu, event);
-}
-
-
 static char *op_name_from_perf_id(enum arm_perf_pmu_ids id)
 {
 	switch (id) {
@@ -175,31 +48,10 @@ static char *op_name_from_perf_id(enum arm_perf_pmu_ids id)
 	}
 }
 
-static int op_arm_create_files(struct super_block *sb, struct dentry *root)
-{
-	unsigned int i;
-
-	for (i = 0; i < perf_num_counters; i++) {
-		struct dentry *dir;
-		char buf[4];
-
-		snprintf(buf, sizeof buf, "%d", i);
-		dir = oprofilefs_mkdir(sb, root, buf);
-		oprofilefs_create_ulong(sb, dir, "enabled", &counter_config[i].enabled);
-		oprofilefs_create_ulong(sb, dir, "event", &counter_config[i].event);
-		oprofilefs_create_ulong(sb, dir, "count", &counter_config[i].count);
-		oprofilefs_create_ulong(sb, dir, "unit_mask", &counter_config[i].unit_mask);
-		oprofilefs_create_ulong(sb, dir, "kernel", &counter_config[i].kernel);
-		oprofilefs_create_ulong(sb, dir, "user", &counter_config[i].user);
-	}
-
-	return 0;
-}
-
 static int op_arm_setup(void)
 {
 	spin_lock(&oprofilefs_lock);
-	op_perf_setup();
+	oprofile_perf_setup();
 	spin_unlock(&oprofilefs_lock);
 	return 0;
 }
@@ -211,7 +63,7 @@ static int op_arm_start(void)
 	mutex_lock(&op_arm_mutex);
 	if (!op_arm_enabled) {
 		ret = 0;
-		op_perf_start();
+		oprofile_perf_start();
 		op_arm_enabled = 1;
 	}
 	mutex_unlock(&op_arm_mutex);
@@ -222,7 +74,7 @@ static void op_arm_stop(void)
 {
 	mutex_lock(&op_arm_mutex);
 	if (op_arm_enabled)
-		op_perf_stop();
+		oprofile_perf_stop();
 	op_arm_enabled = 0;
 	mutex_unlock(&op_arm_mutex);
 }
@@ -232,7 +84,7 @@ static int op_arm_suspend(struct platform_device *dev, pm_message_t state)
 {
 	mutex_lock(&op_arm_mutex);
 	if (op_arm_enabled)
-		op_perf_stop();
+		oprofile_perf_stop();
 	mutex_unlock(&op_arm_mutex);
 	return 0;
 }
@@ -240,7 +92,7 @@ static int op_arm_suspend(struct platform_device *dev, pm_message_t state)
 static int op_arm_resume(struct platform_device *dev)
 {
 	mutex_lock(&op_arm_mutex);
-	if (op_arm_enabled && op_perf_start())
+	if (op_arm_enabled && oprofile_perf_start())
 		op_arm_enabled = 0;
 	mutex_unlock(&op_arm_mutex);
 	return 0;
@@ -351,37 +203,16 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
-	int cpu, ret = 0;
-
-	perf_num_counters = armpmu_get_max_events();
-
-	counter_config = kcalloc(perf_num_counters,
-			sizeof(struct op_counter_config), GFP_KERNEL);
-
-	if (!counter_config) {
-		pr_info("oprofile: failed to allocate %d "
-				"counters\n", perf_num_counters);
-		return -ENOMEM;
-	}
+	int ret = 0;
 
 	ret = init_driverfs();
 	if (ret)
 		return ret;
 
-	for_each_possible_cpu(cpu) {
-		perf_events[cpu] = kcalloc(perf_num_counters,
-				sizeof(struct perf_event *), GFP_KERNEL);
-		if (!perf_events[cpu]) {
-			pr_info("oprofile: failed to allocate %d perf events "
-					"for cpu %d\n", perf_num_counters, cpu);
-			while (--cpu >= 0)
-				kfree(perf_events[cpu]);
-			return -ENOMEM;
-		}
-	}
+	oprofile_perf_set_num_counters(armpmu_get_max_events());
 
 	ops->backtrace		= arm_backtrace;
-	ops->create_files	= op_arm_create_files;
+	ops->create_files	= oprofile_perf_create_files;
 	ops->setup		= op_arm_setup;
 	ops->start		= op_arm_start;
 	ops->stop		= op_arm_stop;
@@ -389,33 +220,21 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 	ops->cpu_type		= op_name_from_perf_id(armpmu_get_pmu_id());
 
 	if (!ops->cpu_type)
-		ret = -ENODEV;
-	else
-		pr_info("oprofile: using %s\n", ops->cpu_type);
+		return -ENODEV;
 
-	return ret;
+	ret = oprofile_perf_init();
+	if (ret != 0)
+		return ret;
+
+	pr_info("oprofile: using %s\n", ops->cpu_type);
+
+	return 0;
 }
 
 void oprofile_arch_exit(void)
 {
-	int cpu, id;
-	struct perf_event *event;
-
+	oprofile_perf_exit();
 	exit_driverfs();
-
-	if (*perf_events) {
-		for_each_possible_cpu(cpu) {
-			for (id = 0; id < perf_num_counters; ++id) {
-				event = perf_events[cpu][id];
-				if (event != NULL)
-					perf_event_release_kernel(event);
-			}
-			kfree(perf_events[cpu]);
-		}
-	}
-
-	if (counter_config)
-		kfree(counter_config);
 }
 #else
 int __init oprofile_arch_init(struct oprofile_operations *ops)
diff --git a/drivers/oprofile/oprofile_perf.c b/drivers/oprofile/oprofile_perf.c
new file mode 100644
index 0000000..5f83cd7
--- /dev/null
+++ b/drivers/oprofile/oprofile_perf.c
@@ -0,0 +1,209 @@
+/*
+ * Copyright 2010 ARM Ltd.
+ *
+ * Perf-events backend for OProfile.
+ */
+#include <linux/slab.h>
+#include <linux/oprofile.h>
+#include <linux/perf_event.h>
+
+/* Per-counter configuration as set via oprofilefs.  */
+struct op_counter_config {
+	unsigned long enabled;
+	unsigned long event;
+
+	unsigned long count;
+
+	/* Dummy values for userspace tool compliance */
+	unsigned long kernel;
+	unsigned long user;
+	unsigned long unit_mask;
+
+	struct perf_event_attr attr;
+};
+
+static struct op_counter_config *counter_config;
+static struct perf_event **perf_events[nr_cpumask_bits];
+static int perf_num_counters;
+
+/*
+ * Overflow callback for oprofile.
+ */
+static void op_overflow_handler(struct perf_event *event, int unused,
+				struct perf_sample_data *data,
+				struct pt_regs *regs)
+{
+	int id;
+	u32 cpu = smp_processor_id();
+
+	for (id = 0; id < perf_num_counters; ++id)
+		if (perf_events[cpu][id] == event)
+			break;
+
+	if (id != perf_num_counters)
+		oprofile_add_sample(regs, id);
+	else
+		pr_warning("oprofile: ignoring spurious overflow "
+				"on cpu %u\n", cpu);
+}
+
+/*
+ * Create perf attributes to mirror the oprofile settings in
+ * counter_config. Attributes are created as `pinned' events and so are
+ * permanently scheduled on the PMU.
+ */
+int oprofile_perf_setup(void)
+{
+	int i;
+	u32 attr_size = sizeof(struct perf_event_attr);
+	struct perf_event_attr *attr;
+
+	for (i = 0; i < perf_num_counters; ++i) {
+		attr = &counter_config[i].attr;
+		memset(attr, 0, attr_size);
+		attr->type		= PERF_TYPE_RAW;
+		attr->size		= attr_size;
+		attr->config		= counter_config[i].event;
+		attr->sample_period	= counter_config[i].count;
+		attr->pinned		= 1;
+	}
+
+	return 0;
+}
+
+int oprofile_create_counter(int cpu, int event)
+{
+	int ret = 0;
+	struct perf_event *pevent;
+
+	if (!counter_config[event].enabled || (perf_events[cpu][event] != NULL))
+		return ret;
+
+	pevent = perf_event_create_kernel_counter(&counter_config[event].attr,
+						  cpu, -1,
+						  op_overflow_handler);
+
+	if (IS_ERR(pevent)) {
+		ret = PTR_ERR(pevent);
+	} else if (pevent->state != PERF_EVENT_STATE_ACTIVE) {
+		pr_warning("oprofile: failed to enable event %d "
+				"on CPU %d\n", event, cpu);
+		ret = -EBUSY;
+	} else {
+		perf_events[cpu][event] = pevent;
+	}
+
+	return ret;
+}
+
+void oprofile_destroy_counter(int cpu, int event)
+{
+	struct perf_event *pevent = perf_events[cpu][event];
+
+	if (pevent) {
+		perf_event_release_kernel(pevent);
+		perf_events[cpu][event] = NULL;
+	}
+}
+
+int oprofile_perf_init(void)
+{
+	u32 counter_size = sizeof(struct op_counter_config);
+	int cpu;
+
+	counter_config = kcalloc(perf_num_counters, counter_size, GFP_KERNEL);
+
+	if (!counter_config) {
+		pr_info("oprofile: failed to allocate %d "
+				"counters\n", perf_num_counters);
+		return -ENOMEM;
+	}
+
+	for_each_possible_cpu(cpu) {
+		perf_events[cpu] = kcalloc(perf_num_counters,
+				sizeof(struct perf_event *), GFP_KERNEL);
+		if (!perf_events[cpu]) {
+			pr_info("oprofile: failed to allocate %d perf events "
+					"for cpu %d\n", perf_num_counters, cpu);
+			while (--cpu >= 0)
+				kfree(perf_events[cpu]);
+			kfree(counter_config);
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
+void oprofile_perf_exit(void)
+{
+	int id, cpu;
+
+	for_each_possible_cpu(cpu) {
+		for (id = 0; id < perf_num_counters; ++id)
+			oprofile_destroy_counter(cpu, id);
+
+		kfree(perf_events[cpu]);
+	}
+
+	kfree(counter_config);
+}
+
+/*
+ * Create active perf events based on the perviously configured
+ * attributes.
+ */
+int oprofile_perf_start(void)
+{
+	int cpu, event, ret = 0;
+
+	for_each_online_cpu(cpu) {
+		for (event = 0; event < perf_num_counters; ++event) {
+			ret = oprofile_create_counter(cpu, event);
+			if (ret)
+				goto out;
+		}
+	}
+
+out:
+	return ret;
+}
+
+/*
+ * Called@the end of a profiling run.
+ */
+void oprofile_perf_stop(void)
+{
+	int cpu, event;
+
+	for_each_online_cpu(cpu)
+		for (event = 0; event < perf_num_counters; ++event)
+			oprofile_destroy_counter(cpu, event);
+}
+
+
+int oprofile_perf_create_files(struct super_block *sb, struct dentry *root)
+{
+	int i;
+
+	for (i = 0; i < perf_num_counters; i++) {
+		struct dentry *dir;
+		char buf[4];
+
+		snprintf(buf, sizeof buf, "%d", i);
+		dir = oprofilefs_mkdir(sb, root, buf);
+		oprofilefs_create_ulong(sb, dir, "enabled", &counter_config[i].enabled);
+		oprofilefs_create_ulong(sb, dir, "event", &counter_config[i].event);
+		oprofilefs_create_ulong(sb, dir, "count", &counter_config[i].count);
+		oprofilefs_create_ulong(sb, dir, "unit_mask", &counter_config[i].unit_mask);
+		oprofilefs_create_ulong(sb, dir, "kernel", &counter_config[i].kernel);
+		oprofilefs_create_ulong(sb, dir, "user", &counter_config[i].user);
+	}
+
+	return 0;
+}
+
+void oprofile_perf_set_num_counters(int num_counters)
+{
+	perf_num_counters = num_counters;
+}
diff --git a/include/linux/oprofile.h b/include/linux/oprofile.h
index 5171639..99e0323 100644
--- a/include/linux/oprofile.h
+++ b/include/linux/oprofile.h
@@ -185,4 +185,16 @@ int oprofile_add_data(struct op_entry *entry, unsigned long val);
 int oprofile_add_data64(struct op_entry *entry, u64 val);
 int oprofile_write_commit(struct op_entry *entry);
 
+/* Oprofile perf wrapper functions. */
+
+#ifdef CONFIG_PERF_EVENTS
+int oprofile_perf_init(void);
+void oprofile_perf_exit(void);
+int oprofile_perf_setup(void);
+int oprofile_perf_start(void);
+void oprofile_perf_stop(void);
+int oprofile_perf_create_files(struct super_block *sb, struct dentry *root);
+void oprofile_perf_set_num_counters(int num_counters);
+#endif /* CONFIG_PERF_EVENTS */
+
 #endif /* OPROFILE_H */
-- 
1.7.1

[PATCH V2 3/4] oprofile: Abstract the perf-events backend

[Will Deacon]

Hi Matt,

I'm still not happy with the init/exit alloc/free code:

On Thu, 2010-08-26 at 20:09 +0100, Matt Fleming wrote:

[...]

> +int oprofile_perf_init(void)
> +{
> +       u32 counter_size = sizeof(struct op_counter_config);
> +       int cpu;
> +
> +       counter_config = kcalloc(perf_num_counters, counter_size, GFP_KERNEL);
> +
> +       if (!counter_config) {
> +               pr_info("oprofile: failed to allocate %d "
> +                               "counters\n", perf_num_counters);
> +               return -ENOMEM;
> +       }
> +
> +       for_each_possible_cpu(cpu) {
> +               perf_events[cpu] = kcalloc(perf_num_counters,
> +                               sizeof(struct perf_event *), GFP_KERNEL);
> +               if (!perf_events[cpu]) {
> +                       pr_info("oprofile: failed to allocate %d perf events "
> +                                       "for cpu %d\n", perf_num_counters, cpu);
> +                       while (--cpu >= 0)
> +                               kfree(perf_events[cpu]);
> +                       kfree(counter_config);
> +                       return -ENOMEM;
> +               }
> +       }
> +
> +       return 0;
> +}

So here, if the perf_events allocation fails for a cpu, we free the
stuff we've already allocated [including counter_config] and return
-ENOMEM. Looking at drivers/oprofile/oprof.c:

static int __init oprofile_init(void)
{
	int err;

	err = oprofile_arch_init(&oprofile_ops);
	if (err < 0 || timer) {
		printk(KERN_INFO "oprofile: using timer interrupt.\n");
		err = oprofile_timer_init(&oprofile_ops);
		if (err)
			goto out_arch;
	}
	err = oprofilefs_register();
	if (err)
		goto out_arch;
	return 0;

out_arch:
	oprofile_arch_exit();
	return err;
}

So now, if timer_init fails or oprofilefs_register fails, we will
call oprofile_arch_exit, which calls oprofile_perf_exit:

> +void oprofile_perf_exit(void)
> +{
> +       int id, cpu;
> +
> +       for_each_possible_cpu(cpu) {
> +               for (id = 0; id < perf_num_counters; ++id)
> +                       oprofile_destroy_counter(cpu, id);
> +
> +               kfree(perf_events[cpu]);
> +       }
> +
> +       kfree(counter_config);
> +}

meaning that we will free everything again! This is what I
was trying to avoid in patch 1/4, by moving the counter_config
freeing into the *_exit function. Looking at it again, I think
all the freeing should be moved to the *_exit function and the init
function should just return error when allocation fails. What do you
think?

> +/*
> + * Create active perf events based on the perviously configured
> + * attributes.
> + */

typo :)

For what it's worth, I tested the series on my Cortex-A9 board and
everything seemed to work fine. I'll give the patches another spin when
we've sorted out these memory issues.

Cheers,

Will

[PATCH V2 3/4] oprofile: Abstract the perf-events backend

[Matt Fleming]

On Fri, Aug 27, 2010 at 11:41:31AM +0100, Will Deacon wrote:
> Hi Matt,
> 
> I'm still not happy with the init/exit alloc/free code:
> 
> On Thu, 2010-08-26 at 20:09 +0100, Matt Fleming wrote:
> 
> [...]
> 
> > +int oprofile_perf_init(void)
> > +{
> > +       u32 counter_size = sizeof(struct op_counter_config);
> > +       int cpu;
> > +
> > +       counter_config = kcalloc(perf_num_counters, counter_size, GFP_KERNEL);
> > +
> > +       if (!counter_config) {
> > +               pr_info("oprofile: failed to allocate %d "
> > +                               "counters\n", perf_num_counters);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       for_each_possible_cpu(cpu) {
> > +               perf_events[cpu] = kcalloc(perf_num_counters,
> > +                               sizeof(struct perf_event *), GFP_KERNEL);
> > +               if (!perf_events[cpu]) {
> > +                       pr_info("oprofile: failed to allocate %d perf events "
> > +                                       "for cpu %d\n", perf_num_counters, cpu);
> > +                       while (--cpu >= 0)
> > +                               kfree(perf_events[cpu]);
> > +                       kfree(counter_config);
> > +                       return -ENOMEM;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> 
> So here, if the perf_events allocation fails for a cpu, we free the
> stuff we've already allocated [including counter_config] and return
> -ENOMEM. Looking at drivers/oprofile/oprof.c:
> 
> static int __init oprofile_init(void)
> {
> 	int err;
> 
> 	err = oprofile_arch_init(&oprofile_ops);
> 	if (err < 0 || timer) {
> 		printk(KERN_INFO "oprofile: using timer interrupt.\n");
> 		err = oprofile_timer_init(&oprofile_ops);
> 		if (err)
> 			goto out_arch;
> 	}
> 	err = oprofilefs_register();
> 	if (err)
> 		goto out_arch;
> 	return 0;
> 
> out_arch:
> 	oprofile_arch_exit();
> 	return err;
> }
> 
> So now, if timer_init fails or oprofilefs_register fails, we will
> call oprofile_arch_exit, which calls oprofile_perf_exit:
> 
> > +void oprofile_perf_exit(void)
> > +{
> > +       int id, cpu;
> > +
> > +       for_each_possible_cpu(cpu) {
> > +               for (id = 0; id < perf_num_counters; ++id)
> > +                       oprofile_destroy_counter(cpu, id);
> > +
> > +               kfree(perf_events[cpu]);
> > +       }
> > +
> > +       kfree(counter_config);
> > +}
> 
> meaning that we will free everything again! This is what I
> was trying to avoid in patch 1/4, by moving the counter_config
> freeing into the *_exit function. Looking at it again, I think
> all the freeing should be moved to the *_exit function and the init
> function should just return error when allocation fails. What do you
> think?

*facepalm*

I dunno how I forgot to fix up that patch. Sorry. You're completely
right, I forgot to role your changes from patch 1/4 into 3/4 when I
shuffled the code around. Thank you for being diligent.

> > +/*
> > + * Create active perf events based on the perviously configured
> > + * attributes.
> > + */
> 
> typo :)

Thanks.

> For what it's worth, I tested the series on my Cortex-A9 board and
> everything seemed to work fine. I'll give the patches another spin when
> we've sorted out these memory issues.

Excellent news. Thanks for testing! I'll get the next version of patch
3/4 out tonight.

[PATCH V2 3/4] oprofile: Abstract the perf-events backend

[Robert Richter]

On 26.08.10 15:09:18, Matt Fleming wrote:
> Move the perf-events backend from arch/arm/oprofile into
> drivers/oprofile so that the code can be shared between architectures.
> 
> This allows each architecture to maintain only a single copy of the
> PMU accessor functions instead of one for both perf and OProfile. It
> also becomes possible for other architectures to delete much of their
> OProfile code in favour of the common code now available in
> drivers/oprofile/oprofile_perf.c.
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
>  arch/arm/oprofile/Makefile       |    4 +
>  arch/arm/oprofile/common.c       |  215 +++-----------------------------------
>  drivers/oprofile/oprofile_perf.c |  209 ++++++++++++++++++++++++++++++++++++
>  include/linux/oprofile.h         |   12 ++
>  4 files changed, 242 insertions(+), 198 deletions(-)
>  create mode 100644 drivers/oprofile/oprofile_perf.c

Could we split this patch in 2 for better review? One that only moves
the code and a 2nd that changes it.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH V2 3/4] oprofile: Abstract the perf-events backend

[Robert Richter]

On 26.08.10 15:09:18, Matt Fleming wrote:
> Move the perf-events backend from arch/arm/oprofile into
> drivers/oprofile so that the code can be shared between architectures.
> 
> This allows each architecture to maintain only a single copy of the
> PMU accessor functions instead of one for both perf and OProfile. It
> also becomes possible for other architectures to delete much of their
> OProfile code in favour of the common code now available in
> drivers/oprofile/oprofile_perf.c.
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
>  arch/arm/oprofile/Makefile       |    4 +
>  arch/arm/oprofile/common.c       |  215 +++-----------------------------------
>  drivers/oprofile/oprofile_perf.c |  209 ++++++++++++++++++++++++++++++++++++
>  include/linux/oprofile.h         |   12 ++
>  4 files changed, 242 insertions(+), 198 deletions(-)
>  create mode 100644 drivers/oprofile/oprofile_perf.c
> 
> diff --git a/arch/arm/oprofile/Makefile b/arch/arm/oprofile/Makefile
> index e666eaf..038d7af 100644
> --- a/arch/arm/oprofile/Makefile
> +++ b/arch/arm/oprofile/Makefile
> @@ -6,4 +6,8 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
>                 oprofilefs.o oprofile_stats.o \
>                 timer_int.o )
> 
> +ifeq ($(CONFIG_HW_PERF_EVENTS), y)
> +DRIVER_OBJS += $(addprefix ../../../drivers/oprofile/, oprofile_perf.o)
> +endif

You use CONFIG_HW_PERF_EVENTS while CONFIG_PERF_EVENTS in oprofile.h.

> +
>  oprofile-y                             := $(DRIVER_OBJS) common.o

> diff --git a/include/linux/oprofile.h b/include/linux/oprofile.h
> index 5171639..99e0323 100644
> --- a/include/linux/oprofile.h
> +++ b/include/linux/oprofile.h
> @@ -185,4 +185,16 @@ int oprofile_add_data(struct op_entry *entry, unsigned long val);
>  int oprofile_add_data64(struct op_entry *entry, u64 val);
>  int oprofile_write_commit(struct op_entry *entry);
> 
> +/* Oprofile perf wrapper functions. */
> +
> +#ifdef CONFIG_PERF_EVENTS
> +int oprofile_perf_init(void);
> +void oprofile_perf_exit(void);
> +int oprofile_perf_setup(void);
> +int oprofile_perf_start(void);
> +void oprofile_perf_stop(void);
> +int oprofile_perf_create_files(struct super_block *sb, struct dentry *root);
> +void oprofile_perf_set_num_counters(int num_counters);
> +#endif /* CONFIG_PERF_EVENTS */

We need empty function stubs for all this functions for the
!CONFIG_PERF_EVENTS case returning -ENODEV or so. Otherwise
compliation will fail for it.

For arm this does not happen because of #ifdefs, but for sh.

Again, please first change the code and then move it without
functional changes, one patch each.

Thanks,

-Robert

> +
>  #endif /* OPROFILE_H */
> --
> 1.7.1
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH V2 4/4] sh: Use the perf-events backend for oprofile

[Matt Fleming]

Use the perf-events based wrapper for oprofile available in
drivers/oprofile. This allows us to centralise the code to control
performance counters.

Signed-off-by: Matt Fleming <matt@console-pimps.org>
---

Paul,

I dropped the CONFIG_PERF_EVENTS dependency from the Makefile in this
version because to do anything useful we need perf events anyway.

 arch/sh/oprofile/Makefile  |    2 +-
 arch/sh/oprofile/common.c  |   96 ++++++++-----------------------------------
 arch/sh/oprofile/op_impl.h |   33 ---------------
 3 files changed, 19 insertions(+), 112 deletions(-)
 delete mode 100644 arch/sh/oprofile/op_impl.h

diff --git a/arch/sh/oprofile/Makefile b/arch/sh/oprofile/Makefile
index 4886c5c..e1015ae 100644
--- a/arch/sh/oprofile/Makefile
+++ b/arch/sh/oprofile/Makefile
@@ -4,6 +4,6 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
 		oprof.o cpu_buffer.o buffer_sync.o \
 		event_buffer.o oprofile_files.o \
 		oprofilefs.o oprofile_stats.o \
-		timer_int.o )
+		timer_int.o oprofile_perf.o )
 
 oprofile-y	:= $(DRIVER_OBJS) common.o backtrace.o
diff --git a/arch/sh/oprofile/common.c b/arch/sh/oprofile/common.c
index ac60493..f8d4a84 100644
--- a/arch/sh/oprofile/common.c
+++ b/arch/sh/oprofile/common.c
@@ -17,71 +17,23 @@
 #include <linux/init.h>
 #include <linux/errno.h>
 #include <linux/smp.h>
+#include <linux/perf_event.h>
 #include <asm/processor.h>
-#include "op_impl.h"
-
-static struct op_sh_model *model;
-
-static struct op_counter_config ctr[20];
 
 extern void sh_backtrace(struct pt_regs * const regs, unsigned int depth);
 
-static int op_sh_setup(void)
-{
-	/* Pre-compute the values to stuff in the hardware registers.  */
-	model->reg_setup(ctr);
-
-	/* Configure the registers on all cpus.  */
-	on_each_cpu(model->cpu_setup, NULL, 1);
-
-        return 0;
-}
-
-static int op_sh_create_files(struct super_block *sb, struct dentry *root)
-{
-	int i, ret = 0;
-
-	for (i = 0; i < model->num_counters; i++) {
-		struct dentry *dir;
-		char buf[4];
-
-		snprintf(buf, sizeof(buf), "%d", i);
-		dir = oprofilefs_mkdir(sb, root, buf);
-
-		ret |= oprofilefs_create_ulong(sb, dir, "enabled", &ctr[i].enabled);
-		ret |= oprofilefs_create_ulong(sb, dir, "event", &ctr[i].event);
-		ret |= oprofilefs_create_ulong(sb, dir, "kernel", &ctr[i].kernel);
-		ret |= oprofilefs_create_ulong(sb, dir, "user", &ctr[i].user);
-
-		if (model->create_files)
-			ret |= model->create_files(sb, dir);
-		else
-			ret |= oprofilefs_create_ulong(sb, dir, "count", &ctr[i].count);
-
-		/* Dummy entries */
-		ret |= oprofilefs_create_ulong(sb, dir, "unit_mask", &ctr[i].unit_mask);
-	}
-
-	return ret;
-}
-
-static int op_sh_start(void)
+static char *op_name_from_perf_name(const char *name)
 {
-	/* Enable performance monitoring for all counters.  */
-	on_each_cpu(model->cpu_start, NULL, 1);
+	if (!strcmp(name, "SH-4A"))
+		return "sh/sh4a";
+	if (!strcmp(name, "SH7750"))
+		return "sh/sh7750";
 
-	return 0;
-}
-
-static void op_sh_stop(void)
-{
-	/* Disable performance monitoring for all counters.  */
-	on_each_cpu(model->cpu_stop, NULL, 1);
+	return NULL;
 }
 
 int __init oprofile_arch_init(struct oprofile_operations *ops)
 {
-	struct op_sh_model *lmodel = NULL;
 	int ret;
 
 	/*
@@ -91,40 +43,28 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
 	 */
 	ops->backtrace = sh_backtrace;
 
-	/*
-	 * XXX
-	 *
-	 * All of the SH7750/SH-4A counters have been converted to perf,
-	 * this infrastructure hook is left for other users until they've
-	 * had a chance to convert over, at which point all of this
-	 * will be deleted.
-	 */
-
-	if (!lmodel)
-		return -ENODEV;
 	if (!(current_cpu_data.flags & CPU_HAS_PERF_COUNTER))
 		return -ENODEV;
 
-	ret = lmodel->init();
-	if (unlikely(ret != 0))
-		return ret;
+	ops->setup		= oprofile_perf_setup;
+	ops->create_files	= oprofile_perf_create_files;
+	ops->start		= oprofile_perf_start;
+	ops->stop		= oprofile_perf_stop;
+	ops->cpu_type		= op_name_from_perf_name(sh_pmu_name());
 
-	model = lmodel;
+	oprofile_perf_set_num_counters(sh_pmu_num_events());
 
-	ops->setup		= op_sh_setup;
-	ops->create_files	= op_sh_create_files;
-	ops->start		= op_sh_start;
-	ops->stop		= op_sh_stop;
-	ops->cpu_type		= lmodel->cpu_type;
+	ret = oprofile_perf_init();
+	if (ret != 0)
+		return ret;
 
 	printk(KERN_INFO "oprofile: using %s performance monitoring.\n",
-	       lmodel->cpu_type);
+	       ops->cpu_type);
 
 	return 0;
 }
 
 void oprofile_arch_exit(void)
 {
-	if (model && model->exit)
-		model->exit();
+	oprofile_perf_exit();
 }
diff --git a/arch/sh/oprofile/op_impl.h b/arch/sh/oprofile/op_impl.h
deleted file mode 100644
index 1244479..0000000
--- a/arch/sh/oprofile/op_impl.h
+++ /dev/null
@@ -1,33 +0,0 @@
-#ifndef __OP_IMPL_H
-#define __OP_IMPL_H
-
-/* Per-counter configuration as set via oprofilefs.  */
-struct op_counter_config {
-	unsigned long enabled;
-	unsigned long event;
-
-	unsigned long count;
-
-	/* Dummy values for userspace tool compliance */
-	unsigned long kernel;
-	unsigned long user;
-	unsigned long unit_mask;
-};
-
-/* Per-architecture configury and hooks.  */
-struct op_sh_model {
-	void (*reg_setup)(struct op_counter_config *);
-	int (*create_files)(struct super_block *sb, struct dentry *dir);
-	void (*cpu_setup)(void *dummy);
-	int (*init)(void);
-	void (*exit)(void);
-	void (*cpu_start)(void *args);
-	void (*cpu_stop)(void *args);
-	char *cpu_type;
-	unsigned char num_counters;
-};
-
-/* arch/sh/oprofile/common.c */
-extern void sh_backtrace(struct pt_regs * const regs, unsigned int depth);
-
-#endif /* __OP_IMPL_H */
-- 
1.7.1

[PATCH V2 4/4] sh: Use the perf-events backend for oprofile

[Robert Richter]

On 26.08.10 15:09:19, Matt Fleming wrote:
> Use the perf-events based wrapper for oprofile available in
> drivers/oprofile. This allows us to centralise the code to control
> performance counters.
> 
> Signed-off-by: Matt Fleming <matt@console-pimps.org>
> ---
> 
> Paul,
> 
> I dropped the CONFIG_PERF_EVENTS dependency from the Makefile in this
> version because to do anything useful we need perf events anyway.

Initialization should simply fail with a printk message for this case,
implement function stubs for the !CONFIG_PERF_EVENTS case instead in
the oprofile.h header file.

> 
>  arch/sh/oprofile/Makefile  |    2 +-
>  arch/sh/oprofile/common.c  |   96 ++++++++-----------------------------------
>  arch/sh/oprofile/op_impl.h |   33 ---------------
>  3 files changed, 19 insertions(+), 112 deletions(-)
>  delete mode 100644 arch/sh/oprofile/op_impl.h
> 
> diff --git a/arch/sh/oprofile/Makefile b/arch/sh/oprofile/Makefile
> index 4886c5c..e1015ae 100644
> --- a/arch/sh/oprofile/Makefile
> +++ b/arch/sh/oprofile/Makefile
> @@ -4,6 +4,6 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
>  		oprof.o cpu_buffer.o buffer_sync.o \
>  		event_buffer.o oprofile_files.o \
>  		oprofilefs.o oprofile_stats.o \
> -		timer_int.o )
> +		timer_int.o oprofile_perf.o )
>  
>  oprofile-y	:= $(DRIVER_OBJS) common.o backtrace.o
> diff --git a/arch/sh/oprofile/common.c b/arch/sh/oprofile/common.c
> index ac60493..f8d4a84 100644
> --- a/arch/sh/oprofile/common.c
> +++ b/arch/sh/oprofile/common.c
> @@ -17,71 +17,23 @@
>  #include <linux/init.h>
>  #include <linux/errno.h>
>  #include <linux/smp.h>
> +#include <linux/perf_event.h>

I don't see a reason why this must be included here.

It's only for sh_pmu_name() and sh_pmu_num_events(), so the interface
looks wrong here. It should be in oprofile_perf.c. The functions
should be generic non-arch perf code. See below.

>  #include <asm/processor.h>
> -#include "op_impl.h"
> -
> -static struct op_sh_model *model;
> -
> -static struct op_counter_config ctr[20];
>  
>  extern void sh_backtrace(struct pt_regs * const regs, unsigned int depth);
>  
> -static int op_sh_setup(void)
> -{
> -	/* Pre-compute the values to stuff in the hardware registers.  */
> -	model->reg_setup(ctr);
> -
> -	/* Configure the registers on all cpus.  */
> -	on_each_cpu(model->cpu_setup, NULL, 1);
> -
> -        return 0;
> -}
> -
> -static int op_sh_create_files(struct super_block *sb, struct dentry *root)
> -{
> -	int i, ret = 0;
> -
> -	for (i = 0; i < model->num_counters; i++) {
> -		struct dentry *dir;
> -		char buf[4];
> -
> -		snprintf(buf, sizeof(buf), "%d", i);
> -		dir = oprofilefs_mkdir(sb, root, buf);
> -
> -		ret |= oprofilefs_create_ulong(sb, dir, "enabled", &ctr[i].enabled);
> -		ret |= oprofilefs_create_ulong(sb, dir, "event", &ctr[i].event);
> -		ret |= oprofilefs_create_ulong(sb, dir, "kernel", &ctr[i].kernel);
> -		ret |= oprofilefs_create_ulong(sb, dir, "user", &ctr[i].user);
> -
> -		if (model->create_files)
> -			ret |= model->create_files(sb, dir);
> -		else
> -			ret |= oprofilefs_create_ulong(sb, dir, "count", &ctr[i].count);
> -
> -		/* Dummy entries */
> -		ret |= oprofilefs_create_ulong(sb, dir, "unit_mask", &ctr[i].unit_mask);
> -	}
> -
> -	return ret;
> -}
> -
> -static int op_sh_start(void)
> +static char *op_name_from_perf_name(const char *name)
>  {
> -	/* Enable performance monitoring for all counters.  */
> -	on_each_cpu(model->cpu_start, NULL, 1);
> +	if (!strcmp(name, "SH-4A"))
> +		return "sh/sh4a";
> +	if (!strcmp(name, "SH7750"))
> +		return "sh/sh7750";

With that implementation we always have to touch the code for new
cpus. Maybe we derive it from the perf name, e.g. making all lowercase
and removing dashes?

>  
> -	return 0;
> -}
> -
> -static void op_sh_stop(void)
> -{
> -	/* Disable performance monitoring for all counters.  */
> -	on_each_cpu(model->cpu_stop, NULL, 1);
> +	return NULL;
>  }
>  
>  int __init oprofile_arch_init(struct oprofile_operations *ops)
>  {
> -	struct op_sh_model *lmodel = NULL;
>  	int ret;
>  
>  	/*
> @@ -91,40 +43,28 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>  	 */
>  	ops->backtrace = sh_backtrace;
>  
> -	/*
> -	 * XXX
> -	 *
> -	 * All of the SH7750/SH-4A counters have been converted to perf,
> -	 * this infrastructure hook is left for other users until they've
> -	 * had a chance to convert over, at which point all of this
> -	 * will be deleted.
> -	 */
> -
> -	if (!lmodel)
> -		return -ENODEV;
>  	if (!(current_cpu_data.flags & CPU_HAS_PERF_COUNTER))
>  		return -ENODEV;
>  
> -	ret = lmodel->init();
> -	if (unlikely(ret != 0))
> -		return ret;
> +	ops->setup		= oprofile_perf_setup;
> +	ops->create_files	= oprofile_perf_create_files;
> +	ops->start		= oprofile_perf_start;
> +	ops->stop		= oprofile_perf_stop;
> +	ops->cpu_type		= op_name_from_perf_name(sh_pmu_name());
>  
> -	model = lmodel;
> +	oprofile_perf_set_num_counters(sh_pmu_num_events());
>  
> -	ops->setup		= op_sh_setup;
> -	ops->create_files	= op_sh_create_files;
> -	ops->start		= op_sh_start;
> -	ops->stop		= op_sh_stop;
> -	ops->cpu_type		= lmodel->cpu_type;
> +	ret = oprofile_perf_init();

Instead of exporting all the functions above implement something like:

	name = op_name_from_perf_name(sh_pmu_name());
	num_events = sh_pmu_num_events();
	ret = oprofile_perf_init(ops, name, num_events);

We will then have only oprofile_perf_init() and oprofile_perf_exit()
as interface which is much cleaner.

-Robert

> +	if (ret != 0)
> +		return ret;
>  
>  	printk(KERN_INFO "oprofile: using %s performance monitoring.\n",
> -	       lmodel->cpu_type);
> +	       ops->cpu_type);
>  
>  	return 0;
>  }
>  
>  void oprofile_arch_exit(void)
>  {
> -	if (model && model->exit)
> -		model->exit();
> +	oprofile_perf_exit();
>  }

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH V2 4/4] sh: Use the perf-events backend for oprofile

[Matt Fleming]

On Fri, Aug 27, 2010 at 04:59:01PM +0200, Robert Richter wrote:
> On 26.08.10 15:09:19, Matt Fleming wrote:
> > Use the perf-events based wrapper for oprofile available in
> > drivers/oprofile. This allows us to centralise the code to control
> > performance counters.
> > 
> > Signed-off-by: Matt Fleming <matt@console-pimps.org>
> > ---
> > 
> > Paul,
> > 
> > I dropped the CONFIG_PERF_EVENTS dependency from the Makefile in this
> > version because to do anything useful we need perf events anyway.
> 
> Initialization should simply fail with a printk message for this case,
> implement function stubs for the !CONFIG_PERF_EVENTS case instead in
> the oprofile.h header file.

I didn't do this because I was hoping that eventually we'd make
CONFIG_OPROFILE select PERF_EVENTS. Would you be OK making that change
instead? Runtime failure is best avoided where possible, especially when
we can sort this out at compile time.

> > 
> >  arch/sh/oprofile/Makefile  |    2 +-
> >  arch/sh/oprofile/common.c  |   96 ++++++++-----------------------------------
> >  arch/sh/oprofile/op_impl.h |   33 ---------------
> >  3 files changed, 19 insertions(+), 112 deletions(-)
> >  delete mode 100644 arch/sh/oprofile/op_impl.h
> > 
> > diff --git a/arch/sh/oprofile/Makefile b/arch/sh/oprofile/Makefile
> > index 4886c5c..e1015ae 100644
> > --- a/arch/sh/oprofile/Makefile
> > +++ b/arch/sh/oprofile/Makefile
> > @@ -4,6 +4,6 @@ DRIVER_OBJS = $(addprefix ../../../drivers/oprofile/, \
> >  		oprof.o cpu_buffer.o buffer_sync.o \
> >  		event_buffer.o oprofile_files.o \
> >  		oprofilefs.o oprofile_stats.o \
> > -		timer_int.o )
> > +		timer_int.o oprofile_perf.o )
> >  
> >  oprofile-y	:= $(DRIVER_OBJS) common.o backtrace.o
> > diff --git a/arch/sh/oprofile/common.c b/arch/sh/oprofile/common.c
> > index ac60493..f8d4a84 100644
> > --- a/arch/sh/oprofile/common.c
> > +++ b/arch/sh/oprofile/common.c
> > @@ -17,71 +17,23 @@
> >  #include <linux/init.h>
> >  #include <linux/errno.h>
> >  #include <linux/smp.h>
> > +#include <linux/perf_event.h>
> 
> I don't see a reason why this must be included here.
> 
> It's only for sh_pmu_name() and sh_pmu_num_events(), so the interface
> looks wrong here. It should be in oprofile_perf.c. The functions
> should be generic non-arch perf code. See below.

These functions are inherently architecture-specific. See my reply to
your response to patch 2/4.

> >  #include <asm/processor.h>
> > -#include "op_impl.h"
> > -
> > -static struct op_sh_model *model;
> > -
> > -static struct op_counter_config ctr[20];
> >  
> >  extern void sh_backtrace(struct pt_regs * const regs, unsigned int depth);
> >  
> > -static int op_sh_setup(void)
> > -{
> > -	/* Pre-compute the values to stuff in the hardware registers.  */
> > -	model->reg_setup(ctr);
> > -
> > -	/* Configure the registers on all cpus.  */
> > -	on_each_cpu(model->cpu_setup, NULL, 1);
> > -
> > -        return 0;
> > -}
> > -
> > -static int op_sh_create_files(struct super_block *sb, struct dentry *root)
> > -{
> > -	int i, ret = 0;
> > -
> > -	for (i = 0; i < model->num_counters; i++) {
> > -		struct dentry *dir;
> > -		char buf[4];
> > -
> > -		snprintf(buf, sizeof(buf), "%d", i);
> > -		dir = oprofilefs_mkdir(sb, root, buf);
> > -
> > -		ret |= oprofilefs_create_ulong(sb, dir, "enabled", &ctr[i].enabled);
> > -		ret |= oprofilefs_create_ulong(sb, dir, "event", &ctr[i].event);
> > -		ret |= oprofilefs_create_ulong(sb, dir, "kernel", &ctr[i].kernel);
> > -		ret |= oprofilefs_create_ulong(sb, dir, "user", &ctr[i].user);
> > -
> > -		if (model->create_files)
> > -			ret |= model->create_files(sb, dir);
> > -		else
> > -			ret |= oprofilefs_create_ulong(sb, dir, "count", &ctr[i].count);
> > -
> > -		/* Dummy entries */
> > -		ret |= oprofilefs_create_ulong(sb, dir, "unit_mask", &ctr[i].unit_mask);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> > -static int op_sh_start(void)
> > +static char *op_name_from_perf_name(const char *name)
> >  {
> > -	/* Enable performance monitoring for all counters.  */
> > -	on_each_cpu(model->cpu_start, NULL, 1);
> > +	if (!strcmp(name, "SH-4A"))
> > +		return "sh/sh4a";
> > +	if (!strcmp(name, "SH7750"))
> > +		return "sh/sh7750";
> 
> With that implementation we always have to touch the code for new
> cpus. Maybe we derive it from the perf name, e.g. making all lowercase
> and removing dashes?

Is this code really that bad that we need to start playing string
manipulation games?

> >  
> > -	return 0;
> > -}
> > -
> > -static void op_sh_stop(void)
> > -{
> > -	/* Disable performance monitoring for all counters.  */
> > -	on_each_cpu(model->cpu_stop, NULL, 1);
> > +	return NULL;
> >  }
> >  
> >  int __init oprofile_arch_init(struct oprofile_operations *ops)
> >  {
> > -	struct op_sh_model *lmodel = NULL;
> >  	int ret;
> >  
> >  	/*
> > @@ -91,40 +43,28 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
> >  	 */
> >  	ops->backtrace = sh_backtrace;
> >  
> > -	/*
> > -	 * XXX
> > -	 *
> > -	 * All of the SH7750/SH-4A counters have been converted to perf,
> > -	 * this infrastructure hook is left for other users until they've
> > -	 * had a chance to convert over, at which point all of this
> > -	 * will be deleted.
> > -	 */
> > -
> > -	if (!lmodel)
> > -		return -ENODEV;
> >  	if (!(current_cpu_data.flags & CPU_HAS_PERF_COUNTER))
> >  		return -ENODEV;
> >  
> > -	ret = lmodel->init();
> > -	if (unlikely(ret != 0))
> > -		return ret;
> > +	ops->setup		= oprofile_perf_setup;
> > +	ops->create_files	= oprofile_perf_create_files;
> > +	ops->start		= oprofile_perf_start;
> > +	ops->stop		= oprofile_perf_stop;
> > +	ops->cpu_type		= op_name_from_perf_name(sh_pmu_name());
> >  
> > -	model = lmodel;
> > +	oprofile_perf_set_num_counters(sh_pmu_num_events());
> >  
> > -	ops->setup		= op_sh_setup;
> > -	ops->create_files	= op_sh_create_files;
> > -	ops->start		= op_sh_start;
> > -	ops->stop		= op_sh_stop;
> > -	ops->cpu_type		= lmodel->cpu_type;
> > +	ret = oprofile_perf_init();
> 
> Instead of exporting all the functions above implement something like:
> 
> 	name = op_name_from_perf_name(sh_pmu_name());
> 	num_events = sh_pmu_num_events();
> 	ret = oprofile_perf_init(ops, name, num_events);
> 
> We will then have only oprofile_perf_init() and oprofile_perf_exit()
> as interface which is much cleaner.

Well, the reason that I left it this way is so that architectures can
choose to implement wrappers around the oprofile_perf_* functions. I
don't think ARM or SH actually need wrappers (the only extra thing that
ARM does is locking which SH should probably do too) but I assumed there
was a reason that these functions pointers were exposed originally. I
haven't look at what other architectures would do. I'll take a look at
that.

[PATCH V2 4/4] sh: Use the perf-events backend for oprofile

[Robert Richter]

On 27.08.10 16:19:46, Matt Fleming wrote:
> On Fri, Aug 27, 2010 at 04:59:01PM +0200, Robert Richter wrote:
> > On 26.08.10 15:09:19, Matt Fleming wrote:
> > > Use the perf-events based wrapper for oprofile available in
> > > drivers/oprofile. This allows us to centralise the code to control
> > > performance counters.
> > > 
> > > Signed-off-by: Matt Fleming <matt@console-pimps.org>
> > > ---
> > > 
> > > Paul,
> > > 
> > > I dropped the CONFIG_PERF_EVENTS dependency from the Makefile in this
> > > version because to do anything useful we need perf events anyway.
> > 
> > Initialization should simply fail with a printk message for this case,
> > implement function stubs for the !CONFIG_PERF_EVENTS case instead in
> > the oprofile.h header file.
> 
> I didn't do this because I was hoping that eventually we'd make
> CONFIG_OPROFILE select PERF_EVENTS. Would you be OK making that change
> instead? Runtime failure is best avoided where possible, especially when
> we can sort this out at compile time.

Ok, we don't need it if we add architectural dependencies to Kconfig
for those architectures requiring perf.

> > > -static int op_sh_start(void)
> > > +static char *op_name_from_perf_name(const char *name)
> > >  {
> > > -	/* Enable performance monitoring for all counters.  */
> > > -	on_each_cpu(model->cpu_start, NULL, 1);
> > > +	if (!strcmp(name, "SH-4A"))
> > > +		return "sh/sh4a";
> > > +	if (!strcmp(name, "SH7750"))
> > > +		return "sh/sh7750";
> > 
> > With that implementation we always have to touch the code for new
> > cpus. Maybe we derive it from the perf name, e.g. making all lowercase
> > and removing dashes?
> 
> Is this code really that bad that we need to start playing string
> manipulation games?

No, but with that implementation we always have to update the cpu
string with each new cpu though nothing else changes. We may keep this
code. But, shouldn't we return a default string "sh/<name>" for all
other cases? We will then need to update only the oprofile userland
with new cpus.

> > > +	ops->setup		= oprofile_perf_setup;
> > > +	ops->create_files	= oprofile_perf_create_files;
> > > +	ops->start		= oprofile_perf_start;
> > > +	ops->stop		= oprofile_perf_stop;
> > > +	ops->cpu_type		= op_name_from_perf_name(sh_pmu_name());
> > >  
> > > -	model = lmodel;
> > > +	oprofile_perf_set_num_counters(sh_pmu_num_events());
> > >  
> > > -	ops->setup		= op_sh_setup;
> > > -	ops->create_files	= op_sh_create_files;
> > > -	ops->start		= op_sh_start;
> > > -	ops->stop		= op_sh_stop;
> > > -	ops->cpu_type		= lmodel->cpu_type;
> > > +	ret = oprofile_perf_init();
> > 
> > Instead of exporting all the functions above implement something like:
> > 
> > 	name = op_name_from_perf_name(sh_pmu_name());
> > 	num_events = sh_pmu_num_events();
> > 	ret = oprofile_perf_init(ops, name, num_events);
> > 
> > We will then have only oprofile_perf_init() and oprofile_perf_exit()
> > as interface which is much cleaner.
> 
> Well, the reason that I left it this way is so that architectures can
> choose to implement wrappers around the oprofile_perf_* functions. I
> don't think ARM or SH actually need wrappers (the only extra thing that
> ARM does is locking which SH should probably do too) but I assumed there
> was a reason that these functions pointers were exposed originally. I
> haven't look at what other architectures would do. I'll take a look at
> that.

I am not sure if we need such wrappers, and if so we could implement
it anyway, e.g.:

 oprofile_perf_init(perf_ops, name, num_events);

 op_sh_setup():

	/* setup something */
	...

	perf_ops->setup();

	/* setup more */
	...

But I don't think we need this. And the above makes the interface much
cleaner.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH V2 4/4] sh: Use the perf-events backend for oprofile

[Matt Fleming]

On Tue, Aug 31, 2010 at 01:28:41PM +0200, Robert Richter wrote:
> On 27.08.10 16:19:46, Matt Fleming wrote:
> > On Fri, Aug 27, 2010 at 04:59:01PM +0200, Robert Richter wrote:
> > > On 26.08.10 15:09:19, Matt Fleming wrote:
> > > > Use the perf-events based wrapper for oprofile available in
> > > > drivers/oprofile. This allows us to centralise the code to control
> > > > performance counters.
> > > > 
> > > > Signed-off-by: Matt Fleming <matt@console-pimps.org>
> > > > ---
> > > > 
> > > > Paul,
> > > > 
> > > > I dropped the CONFIG_PERF_EVENTS dependency from the Makefile in this
> > > > version because to do anything useful we need perf events anyway.
> > > 
> > > Initialization should simply fail with a printk message for this case,
> > > implement function stubs for the !CONFIG_PERF_EVENTS case instead in
> > > the oprofile.h header file.
> > 
> > I didn't do this because I was hoping that eventually we'd make
> > CONFIG_OPROFILE select PERF_EVENTS. Would you be OK making that change
> > instead? Runtime failure is best avoided where possible, especially when
> > we can sort this out at compile time.
> 
> Ok, we don't need it if we add architectural dependencies to Kconfig
> for those architectures requiring perf.

OK cool.

> > > > -static int op_sh_start(void)
> > > > +static char *op_name_from_perf_name(const char *name)
> > > >  {
> > > > -	/* Enable performance monitoring for all counters.  */
> > > > -	on_each_cpu(model->cpu_start, NULL, 1);
> > > > +	if (!strcmp(name, "SH-4A"))
> > > > +		return "sh/sh4a";
> > > > +	if (!strcmp(name, "SH7750"))
> > > > +		return "sh/sh7750";
> > > 
> > > With that implementation we always have to touch the code for new
> > > cpus. Maybe we derive it from the perf name, e.g. making all lowercase
> > > and removing dashes?
> > 
> > Is this code really that bad that we need to start playing string
> > manipulation games?
> 
> No, but with that implementation we always have to update the cpu
> string with each new cpu though nothing else changes. We may keep this
> code. But, shouldn't we return a default string "sh/<name>" for all
> other cases? We will then need to update only the oprofile userland
> with new cpus.

These names are actually the names of types of performance counters,
not a specific cpu. All SH-4 cpus that have performance counters have
7750-style performance counters and all SH-4A cpus have SH-4A-style
counters.

It's unlikely we'd have to update this code in the near future. Paul,
correct me if I'm wrong here.

> > > > +	ops->setup		= oprofile_perf_setup;
> > > > +	ops->create_files	= oprofile_perf_create_files;
> > > > +	ops->start		= oprofile_perf_start;
> > > > +	ops->stop		= oprofile_perf_stop;
> > > > +	ops->cpu_type		= op_name_from_perf_name(sh_pmu_name());
> > > >  
> > > > -	model = lmodel;
> > > > +	oprofile_perf_set_num_counters(sh_pmu_num_events());
> > > >  
> > > > -	ops->setup		= op_sh_setup;
> > > > -	ops->create_files	= op_sh_create_files;
> > > > -	ops->start		= op_sh_start;
> > > > -	ops->stop		= op_sh_stop;
> > > > -	ops->cpu_type		= lmodel->cpu_type;
> > > > +	ret = oprofile_perf_init();
> > > 
> > > Instead of exporting all the functions above implement something like:
> > > 
> > > 	name = op_name_from_perf_name(sh_pmu_name());
> > > 	num_events = sh_pmu_num_events();
> > > 	ret = oprofile_perf_init(ops, name, num_events);
> > > 
> > > We will then have only oprofile_perf_init() and oprofile_perf_exit()
> > > as interface which is much cleaner.
> > 
> > Well, the reason that I left it this way is so that architectures can
> > choose to implement wrappers around the oprofile_perf_* functions. I
> > don't think ARM or SH actually need wrappers (the only extra thing that
> > ARM does is locking which SH should probably do too) but I assumed there
> > was a reason that these functions pointers were exposed originally. I
> > haven't look at what other architectures would do. I'll take a look at
> > that.
> 
> I am not sure if we need such wrappers, and if so we could implement
> it anyway, e.g.:
> 
>  oprofile_perf_init(perf_ops, name, num_events);
> 
>  op_sh_setup():
> 
> 	/* setup something */
> 	...
> 
> 	perf_ops->setup();
> 
> 	/* setup more */
> 	...
> 
> But I don't think we need this. And the above makes the interface much
> cleaner.

OK, seeing as the two architectures that will use this initially don't
require wrappers I've no problem doing it your way. It can always be
extended later if necessary. And more importantly, with a proper
usecase we'll be able to see exactly _how_ it needs to be extended.

[PATCH V2 4/4] sh: Use the perf-events backend for oprofile

[Robert Richter]

On 31.08.10 08:23:43, Matt Fleming wrote:
> > > > > -static int op_sh_start(void)
> > > > > +static char *op_name_from_perf_name(const char *name)
> > > > >  {
> > > > > -	/* Enable performance monitoring for all counters.  */
> > > > > -	on_each_cpu(model->cpu_start, NULL, 1);
> > > > > +	if (!strcmp(name, "SH-4A"))
> > > > > +		return "sh/sh4a";
> > > > > +	if (!strcmp(name, "SH7750"))
> > > > > +		return "sh/sh7750";
> > > > 
> > > > With that implementation we always have to touch the code for new
> > > > cpus. Maybe we derive it from the perf name, e.g. making all lowercase
> > > > and removing dashes?
> > > 
> > > Is this code really that bad that we need to start playing string
> > > manipulation games?
> > 
> > No, but with that implementation we always have to update the cpu
> > string with each new cpu though nothing else changes. We may keep this
> > code. But, shouldn't we return a default string "sh/<name>" for all
> > other cases? We will then need to update only the oprofile userland
> > with new cpus.
> 
> These names are actually the names of types of performance counters,
> not a specific cpu. All SH-4 cpus that have performance counters have
> 7750-style performance counters and all SH-4A cpus have SH-4A-style
> counters.
> 
> It's unlikely we'd have to update this code in the near future. Paul,
> correct me if I'm wrong here.

Ok, this shouldn't block this patch series, we still can make a patch
if there is a use case.

> > > > > +	ops->setup		= oprofile_perf_setup;
> > > > > +	ops->create_files	= oprofile_perf_create_files;
> > > > > +	ops->start		= oprofile_perf_start;
> > > > > +	ops->stop		= oprofile_perf_stop;
> > > > > +	ops->cpu_type		= op_name_from_perf_name(sh_pmu_name());
> > > > >  
> > > > > -	model = lmodel;
> > > > > +	oprofile_perf_set_num_counters(sh_pmu_num_events());
> > > > >  
> > > > > -	ops->setup		= op_sh_setup;
> > > > > -	ops->create_files	= op_sh_create_files;
> > > > > -	ops->start		= op_sh_start;
> > > > > -	ops->stop		= op_sh_stop;
> > > > > -	ops->cpu_type		= lmodel->cpu_type;
> > > > > +	ret = oprofile_perf_init();
> > > > 
> > > > Instead of exporting all the functions above implement something like:
> > > > 
> > > > 	name = op_name_from_perf_name(sh_pmu_name());
> > > > 	num_events = sh_pmu_num_events();
> > > > 	ret = oprofile_perf_init(ops, name, num_events);
> > > > 
> > > > We will then have only oprofile_perf_init() and oprofile_perf_exit()
> > > > as interface which is much cleaner.
> > > 
> > > Well, the reason that I left it this way is so that architectures can
> > > choose to implement wrappers around the oprofile_perf_* functions. I
> > > don't think ARM or SH actually need wrappers (the only extra thing that
> > > ARM does is locking which SH should probably do too) but I assumed there
> > > was a reason that these functions pointers were exposed originally. I
> > > haven't look at what other architectures would do. I'll take a look at
> > > that.
> > 
> > I am not sure if we need such wrappers, and if so we could implement
> > it anyway, e.g.:
> > 
> >  oprofile_perf_init(perf_ops, name, num_events);
> > 
> >  op_sh_setup():
> > 
> > 	/* setup something */
> > 	...
> > 
> > 	perf_ops->setup();
> > 
> > 	/* setup more */
> > 	...
> > 
> > But I don't think we need this. And the above makes the interface much
> > cleaner.
> 
> OK, seeing as the two architectures that will use this initially don't
> require wrappers I've no problem doing it your way. It can always be
> extended later if necessary. And more importantly, with a proper
> usecase we'll be able to see exactly _how_ it needs to be extended.

Yes, right. So I am looking forward to your new version.

Thanks,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH V2 0/4] Generalise ARM perf-events backend for oprofile

[Robert Richter]

On 26.08.10 15:09:15, 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.
> 
> 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. Though, I have been able to compile this version of the
> series.
> 
> This patch series is based on tip/master.

Matt,

please rebase your next version of this patch set to

 git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git core

It contains Will's patches.

Thanks,

-Robert

> 
> Changes from v1:
>  - Prefix the new functons with "oprofile_" instead of "op_".
>  - Fix ARM compilation errors
>  - Move all the oprofile-perf logic into oprofile_perf.c
>  - Include cleanup patch from Will
> 
> Matt Fleming (3):
>   sh: Accessor functions for the sh_pmu state
>   oprofile: Abstract the perf-events backend
>   sh: Use the perf-events backend for oprofile
> 
> Will Deacon (1):
>   oprofile: Handle initialisation failure more gracefully
> 
>  arch/arm/oprofile/Makefile       |    4 +
>  arch/arm/oprofile/common.c       |  228 ++++----------------------------------
>  arch/sh/include/asm/perf_event.h |    2 +
>  arch/sh/kernel/perf_event.c      |   13 ++
>  arch/sh/oprofile/Makefile        |    2 +-
>  arch/sh/oprofile/common.c        |   96 +++-------------
>  arch/sh/oprofile/op_impl.h       |   33 ------
>  drivers/oprofile/oprofile_perf.c |  209 ++++++++++++++++++++++++++++++++++
>  include/linux/oprofile.h         |   12 ++
>  9 files changed, 283 insertions(+), 316 deletions(-)
>  delete mode 100644 arch/sh/oprofile/op_impl.h
>  create mode 100644 drivers/oprofile/oprofile_perf.c
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH V2 0/4] Generalise ARM perf-events backend for oprofile

[Matt Fleming]

On Tue, Aug 31, 2010 at 01:05:17PM +0200, Robert Richter wrote:
> On 26.08.10 15:09:15, 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.
> > 
> > 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. Though, I have been able to compile this version of the
> > series.
> > 
> > This patch series is based on tip/master.
> 
> Matt,
> 
> please rebase your next version of this patch set to
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git core
> 
> It contains Will's patches.

Sure, will do! Thanks.