Re: [Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-26 Thread K.Prasad 
On Tue, May 25, 2010 at 06:39:19AM -0500, Millton Miller wrote:
 On Tue, 25 May 2010 at 14:43:56 +0530, K.Prasad wrote:
  Certain architectures (such as PowerPC Book III S) have a need to cleanup
  data-structures before the breakpoint is unregistered. This patch introduces
  an arch-specific hook in release_bp_slot() along with a weak definition in
  the form of a stub funciton.
  
  Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
  ---
   kernel/hw_breakpoint.c |   12 
   1 file changed, 12 insertions(+)
 
 
 My understanding is weak function definitions must appear in a different C
 file than their call sites to work on some toolchains.
 

Atleast, there are quite a few precedents inside the Linux kernel for
__weak functions being invoked from the file in which they are defined
(arch_hwblk_init, arch_enable_nonboot_cpus_begin and hw_perf_disable to
name a few).
Moreover the online GCC docs haven't any such constraints mentioned.

 Andrew, can you confirm the above statement?
 
  Index: linux-2.6.ppc64_test/kernel/hw_breakpoint.c
  ===
  --- linux-2.6.ppc64_test.orig/kernel/hw_breakpoint.c
  +++ linux-2.6.ppc64_test/kernel/hw_breakpoint.c
  @@ -242,6 +242,17 @@  toggle_bp_slot(struct perf_event *bp, bo
   }
   
   /*
  + * Function to perform processor-specific cleanup during unregistration
  + */
  +__weak void arch_unregister_hw_breakpoint(struct perf_event *bp)
  +{
  +   /*
  +* A weak stub function here for those archs that don't define
  +* it inside arch/.../kernel/hw_breakpoint.c
  +*/
  +}
  +
  +/*
* Contraints to check before allowing this new breakpoint counter:
*
*  == Non-pinned counter == (Considered as pinned for now)
  @@ -339,6 +350,7 @@  void release_bp_slot(struct perf_event *
   {
  mutex_lock(nr_bp_mutex);
   
  +   arch_unregister_hw_breakpoint(bp);
  __release_bp_slot(bp);
   
  mutex_unlock(nr_bp_mutex);
  
 
 
 Since the weak version is empty, should it just be delcared (in
 a header, put the comment there) and not defined?


The initial thinking behind defining it in the .c file was, for one,
the function need not be moved (from .h to .c) when other architectures
have a need to populate them. Secondly, given that powerpc (which has a
'strong' definition for arch_unregister_hw_breakpoint()) includes the
header file (in which this can be moved to) I wasn't sure about
possible conflicts.

 milton
 ___
 Linuxppc-dev mailing list
 Linuxppc-dev@lists.ozlabs.org
 https://lists.ozlabs.org/listinfo/linuxppc-dev

Thanks,
K.Prasad

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-26 Thread David Howells 
K.Prasad pra...@linux.vnet.ibm.com wrote:

  My understanding is weak function definitions must appear in a different C
  file than their call sites to work on some toolchains.
  
 
 Atleast, there are quite a few precedents inside the Linux kernel for
 __weak functions being invoked from the file in which they are defined
 (arch_hwblk_init, arch_enable_nonboot_cpus_begin and hw_perf_disable to
 name a few).
 Moreover the online GCC docs haven't any such constraints mentioned.

I've seen problems in this area.  gcc sometimes inlines a weak function that's
in the same file as the call point.

David
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-26 Thread Michael Ellerman 
On Wed, 2010-05-26 at 10:54 +0100, David Howells wrote:
 K.Prasad pra...@linux.vnet.ibm.com wrote:
 
   My understanding is weak function definitions must appear in a different C
   file than their call sites to work on some toolchains.
   
  
  Atleast, there are quite a few precedents inside the Linux kernel for
  __weak functions being invoked from the file in which they are defined
  (arch_hwblk_init, arch_enable_nonboot_cpus_begin and hw_perf_disable to
  name a few).
  Moreover the online GCC docs haven't any such constraints mentioned.
 
 I've seen problems in this area.  gcc sometimes inlines a weak function that's
 in the same file as the call point.

See the functions in kernel/softirq.c for example, and commits 43a256322
and b2e2fe996 - though unhelpfully they don't mention the gcc version. A
bit of googling suggests it was probably gcc version 4.1.1 20060525
(Red Hat 4.1.1-1) in that case.

But the example of hw_perf_enable() (which is weak in the same unit),
suggests maybe this isn't a bug many people are hitting in practice
anymore.

Having said that the #define foo foo pattern is reasonably neat and
avoids the problem altogether, see eg. arch_setup_msi_irqs.

cheers


signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-26 Thread K.Prasad 
On Wed, May 26, 2010 at 10:54:41AM +0100, David Howells wrote:
 K.Prasad pra...@linux.vnet.ibm.com wrote:
 
   My understanding is weak function definitions must appear in a different C
   file than their call sites to work on some toolchains.
   
  
  Atleast, there are quite a few precedents inside the Linux kernel for
  __weak functions being invoked from the file in which they are defined
  (arch_hwblk_init, arch_enable_nonboot_cpus_begin and hw_perf_disable to
  name a few).
  Moreover the online GCC docs haven't any such constraints mentioned.
 
 I've seen problems in this area.  gcc sometimes inlines a weak function that's
 in the same file as the call point.
 

We've seen such behaviour even otherwiseeven with noinline attribute
in place. I'm not sure if this gcc fix
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=16922) helped correct the
behaviour, but the lesson has been to not trust a function to be
inlined/remain non-inline consistently.

 David

Thanks,
K.Prasad
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-26 Thread Frederic Weisbecker 
On Wed, May 26, 2010 at 10:47:42PM +0530, K.Prasad wrote:
 On Wed, May 26, 2010 at 10:54:41AM +0100, David Howells wrote:
  K.Prasad pra...@linux.vnet.ibm.com wrote:
  
My understanding is weak function definitions must appear in a 
different C
file than their call sites to work on some toolchains.

   
   Atleast, there are quite a few precedents inside the Linux kernel for
   __weak functions being invoked from the file in which they are defined
   (arch_hwblk_init, arch_enable_nonboot_cpus_begin and hw_perf_disable to
   name a few).
   Moreover the online GCC docs haven't any such constraints mentioned.
  
  I've seen problems in this area.  gcc sometimes inlines a weak function 
  that's
  in the same file as the call point.
  
 
 We've seen such behaviour even otherwiseeven with noinline attribute
 in place. I'm not sure if this gcc fix
 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=16922) helped correct the
 behaviour, but the lesson has been to not trust a function to be
 inlined/remain non-inline consistently.


If we can't put the call to the function in the same file of its weak
definition, then perf is totally screwed.

And in fact it makes __weak basically useless and unusable. I guess
that happened in old gcc versions that have been fixed now.

Anyway, I'm personally fine with this patch (you can put my hack
if you want).

Thanks.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-26 Thread K.Prasad 
On Wed, May 26, 2010 at 10:47:42PM +0530, K.Prasad wrote:
 On Wed, May 26, 2010 at 10:54:41AM +0100, David Howells wrote:
  K.Prasad pra...@linux.vnet.ibm.com wrote:
  
My understanding is weak function definitions must appear in a 
different C
file than their call sites to work on some toolchains.

   
   Atleast, there are quite a few precedents inside the Linux kernel for
   __weak functions being invoked from the file in which they are defined
   (arch_hwblk_init, arch_enable_nonboot_cpus_begin and hw_perf_disable to
   name a few).
   Moreover the online GCC docs haven't any such constraints mentioned.
  
  I've seen problems in this area.  gcc sometimes inlines a weak function 
  that's
  in the same file as the call point.
  
 
 We've seen such behaviour even otherwiseeven with noinline attribute
 in place. I'm not sure if this gcc fix
 (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=16922) helped correct the

Looks like I cited the wrong bug. The appropriate one is
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=34563.

Thanks,
K.Prasad

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-26 Thread K.Prasad 
On Wed, May 26, 2010 at 07:23:15PM +0200, Frederic Weisbecker wrote:
 On Wed, May 26, 2010 at 10:47:42PM +0530, K.Prasad wrote:
  On Wed, May 26, 2010 at 10:54:41AM +0100, David Howells wrote:
   K.Prasad pra...@linux.vnet.ibm.com wrote:
   
 My understanding is weak function definitions must appear in a 
 different C
 file than their call sites to work on some toolchains.
 

Atleast, there are quite a few precedents inside the Linux kernel for
__weak functions being invoked from the file in which they are defined
(arch_hwblk_init, arch_enable_nonboot_cpus_begin and hw_perf_disable to
name a few).
Moreover the online GCC docs haven't any such constraints mentioned.
   
   I've seen problems in this area.  gcc sometimes inlines a weak function 
   that's
   in the same file as the call point.
   
  
  We've seen such behaviour even otherwiseeven with noinline attribute
  in place. I'm not sure if this gcc fix
  (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=16922) helped correct the
  behaviour, but the lesson has been to not trust a function to be
  inlined/remain non-inline consistently.
 
 
 If we can't put the call to the function in the same file of its weak
 definition, then perf is totally screwed.
 
 And in fact it makes __weak basically useless and unusable. I guess
 that happened in old gcc versions that have been fixed now.
 
 Anyway, I'm personally fine with this patch (you can put my hack
 if you want).


I guess you meant Acked-by: :-)

Thanks, I'll add the same.

--K.Prasad
 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-26 Thread Frederic Weisbecker 
On Wed, May 26, 2010 at 11:01:24PM +0530, K.Prasad wrote:
 On Wed, May 26, 2010 at 07:23:15PM +0200, Frederic Weisbecker wrote:
  On Wed, May 26, 2010 at 10:47:42PM +0530, K.Prasad wrote:
   On Wed, May 26, 2010 at 10:54:41AM +0100, David Howells wrote:
K.Prasad pra...@linux.vnet.ibm.com wrote:

  My understanding is weak function definitions must appear in a 
  different C
  file than their call sites to work on some toolchains.
  
 
 Atleast, there are quite a few precedents inside the Linux kernel for
 __weak functions being invoked from the file in which they are defined
 (arch_hwblk_init, arch_enable_nonboot_cpus_begin and hw_perf_disable 
 to
 name a few).
 Moreover the online GCC docs haven't any such constraints mentioned.

I've seen problems in this area.  gcc sometimes inlines a weak function 
that's
in the same file as the call point.

   
   We've seen such behaviour even otherwiseeven with noinline attribute
   in place. I'm not sure if this gcc fix
   (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=16922) helped correct the
   behaviour, but the lesson has been to not trust a function to be
   inlined/remain non-inline consistently.
  
  
  If we can't put the call to the function in the same file of its weak
  definition, then perf is totally screwed.
  
  And in fact it makes __weak basically useless and unusable. I guess
  that happened in old gcc versions that have been fixed now.
  
  Anyway, I'm personally fine with this patch (you can put my hack
  if you want).
 
 
 I guess you meant Acked-by: :-)



Oops, right :)

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-25 Thread K.Prasad 
Certain architectures (such as PowerPC Book III S) have a need to cleanup
data-structures before the breakpoint is unregistered. This patch introduces
an arch-specific hook in release_bp_slot() along with a weak definition in
the form of a stub funciton.

Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
---
 kernel/hw_breakpoint.c |   12 
 1 file changed, 12 insertions(+)

Index: linux-2.6.ppc64_test/kernel/hw_breakpoint.c
===
--- linux-2.6.ppc64_test.orig/kernel/hw_breakpoint.c
+++ linux-2.6.ppc64_test/kernel/hw_breakpoint.c
@@ -242,6 +242,17 @@ toggle_bp_slot(struct perf_event *bp, bo
 }
 
 /*
+ * Function to perform processor-specific cleanup during unregistration
+ */
+__weak void arch_unregister_hw_breakpoint(struct perf_event *bp)
+{
+   /*
+* A weak stub function here for those archs that don't define
+* it inside arch/.../kernel/hw_breakpoint.c
+*/
+}
+
+/*
  * Contraints to check before allowing this new breakpoint counter:
  *
  *  == Non-pinned counter == (Considered as pinned for now)
@@ -339,6 +350,7 @@ void release_bp_slot(struct perf_event *
 {
mutex_lock(nr_bp_mutex);
 
+   arch_unregister_hw_breakpoint(bp);
__release_bp_slot(bp);
 
mutex_unlock(nr_bp_mutex);

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[Patch 1/4] Allow arch-specific cleanup before breakpoint unregistration

2010-05-24 Thread K.Prasad 
Certain architectures (such as PowerPC Book III S) have a need to cleanup
data-structures before the breakpoint is unregistered. This patch introduces
an arch-specific hook in release_bp_slot() along with a weak definition in
the form of a stub funciton.

Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
---
 kernel/hw_breakpoint.c |   12 
 1 file changed, 12 insertions(+)

Index: linux-2.6.ppc64_test/kernel/hw_breakpoint.c
===
--- linux-2.6.ppc64_test.orig/kernel/hw_breakpoint.c
+++ linux-2.6.ppc64_test/kernel/hw_breakpoint.c
@@ -242,6 +242,17 @@ toggle_bp_slot(struct perf_event *bp, bo
 }
 
 /*
+ * Function to perform processor-specific cleanup during unregistration
+ */
+__weak void arch_unregister_hw_breakpoint(struct perf_event *bp)
+{
+   /*
+* A weak stub function here for those archs that don't define
+* it inside arch/.../kernel/hw_breakpoint.c
+*/
+}
+
+/*
  * Contraints to check before allowing this new breakpoint counter:
  *
  *  == Non-pinned counter == (Considered as pinned for now)
@@ -339,6 +350,7 @@ void release_bp_slot(struct perf_event *
 {
mutex_lock(nr_bp_mutex);
 
+   arch_unregister_hw_breakpoint(bp);
__release_bp_slot(bp);
 
mutex_unlock(nr_bp_mutex);

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev