Allow arch-specific cleanup before breakpoint unregistration
in [Kernel]
|
Prev: kbuild: menuconfig with O=<dir> creates empty include/config in base
Next: ath: Failed to stop TX DMA in 100 msec after killing last frame
From: K.Prasad on 26 May 2010 03:00 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(a)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(a)lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev Thanks, K.Prasad -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: K.Prasad on 26 May 2010 13:20 On Wed, May 26, 2010 at 10:54:41AM +0100, David Howells wrote: > K.Prasad <prasad(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: K.Prasad on 26 May 2010 13:30 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(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 26 May 2010 13:30 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(a)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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 26 May 2010 13:40
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(a)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 :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |