| Message ID | 20100525091356.GB29003@in.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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.
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
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
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 :)
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);
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(+)