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

Submitted by K.Prasad on May 25, 2010, 9:13 a.m.

Details

Message ID 20100525091356.GB29003@in.ibm.com
State Superseded
Headers show

Commit Message

K.Prasad May 25, 2010, 9:13 a.m.
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 <prasad@linux.vnet.ibm.com>
---
 kernel/hw_breakpoint.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Milton Miller May 25, 2010, 11:39 a.m.
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 <prasad@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.

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?

milton
K.Prasad May 26, 2010, 6:51 a.m.
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 <prasad@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
David Howells May 26, 2010, 9:54 a.m.
K.Prasad <prasad@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
Michael Ellerman May 26, 2010, 3:13 p.m.
On Wed, 2010-05-26 at 10:54 +0100, David Howells wrote:
> K.Prasad <prasad@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
K.Prasad May 26, 2010, 5:17 p.m.
On Wed, May 26, 2010 at 10:54:41AM +0100, David Howells wrote:
> K.Prasad <prasad@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 otherwise....even 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
Frédéric Weisbecker May 26, 2010, 5:23 p.m.
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 <prasad@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 otherwise....even 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.
K.Prasad May 26, 2010, 5:28 p.m.
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 <prasad@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 otherwise....even 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
K.Prasad May 26, 2010, 5:31 p.m.
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 <prasad@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 otherwise....even 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
Frédéric Weisbecker May 26, 2010, 5:35 p.m.
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 <prasad@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 otherwise....even 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 :)

Patch hide | download patch | download mbox

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