Build failure with latest -git: btrfs on ppc64

Build failure with latest -git: btrfs on ppc64

[Chuck Ebbert]

fs/btrfs/locking.c: In function 'btrfs_path_lock_waiting':
fs/btrfs/locking.c:254: error: implicit declaration of function '__raw_spin_is_contended'

Looks like spin_is_contended() is only available on mips and x86??

$ grep -R spin_is_contended *
arch/x86/include/asm/spinlock.h:static inline int __ticket_spin_is_contended(raw_spinlock_t *lock)
arch/x86/include/asm/spinlock.h: * __byte_spin_is_contended.
arch/x86/include/asm/spinlock.h:static inline int __byte_spin_is_contended(raw_spinlock_t *lock)
arch/x86/include/asm/spinlock.h:static inline int __raw_spin_is_contended(raw_spinlock_t *lock)
arch/x86/include/asm/spinlock.h:	return __ticket_spin_is_contended(lock);
arch/x86/include/asm/paravirt.h:	int (*spin_is_contended)(struct raw_spinlock *lock);
arch/x86/include/asm/paravirt.h:static inline int __raw_spin_is_contended(struct raw_spinlock *lock)
arch/x86/include/asm/paravirt.h:	return PVOP_CALL1(int, pv_lock_ops.spin_is_contended, lock);
arch/x86/kernel/paravirt-spinlocks.c:	.spin_is_contended = __ticket_spin_is_contended,
arch/x86/kernel/paravirt-spinlocks.c:	pv_lock_ops.spin_is_contended = __byte_spin_is_contended;
arch/x86/xen/spinlock.c:static int xen_spin_is_contended(struct raw_spinlock *lock)
arch/x86/xen/spinlock.c:	pv_lock_ops.spin_is_contended = xen_spin_is_contended;
arch/mips/include/asm/spinlock.h:static inline int __raw_spin_is_contended(raw_spinlock_t *lock)
fs/btrfs/locking.c:		if (spin_is_contended(&eb->lock) ||
include/linux/spinlock.h:#define spin_is_contended(lock) ((lock)->break_lock)
include/linux/spinlock.h:#define spin_is_contended(lock)	__raw_spin_is_contended(&(lock)->raw_lock)
include/linux/sched.h:	return spin_is_contended(lock);
include/linux/spinlock_up.h:#define __raw_spin_is_contended(lock)	(((void)(lock), 0))

Re: Build failure with latest -git: btrfs on ppc64

[Kyle McMartin]

On Sat, Feb 07, 2009 at 03:50:39PM -0500, Chuck Ebbert wrote:
> fs/btrfs/locking.c: In function 'btrfs_path_lock_waiting':
> fs/btrfs/locking.c:254: error: implicit declaration of function '__raw_spin_is_contended'
> 
> Looks like spin_is_contended() is only available on mips and x86??
> 

Looks to be if there are other folks in line for the ticket lock.

Arguably this should probably be something silly in spinlock.h like,

#ifndef __raw_spin_is_contended
# define spin_is_contended (0)
#else
# define spin_is_contended(x) __raw_spin_is_contended((x))
#endif

or something since in theory anybody has a chance of acquiring it next
(well, lies because caches bias, but there's no waiters at least.)

Either that, or we just define it to be spin_is_locked if the assumption
is that the lock will continue to be locked...

regards, Kyle

Re: Build failure with latest -git: btrfs on ppc64

[Chris Mason]

On Sat, 2009-02-07 at 18:24 -0500, Kyle McMartin wrote:
> On Sat, Feb 07, 2009 at 03:50:39PM -0500, Chuck Ebbert wrote:
> > fs/btrfs/locking.c: In function 'btrfs_path_lock_waiting':
> > fs/btrfs/locking.c:254: error: implicit declaration of function '__raw_spin_is_contended'
> > 
> > Looks like spin_is_contended() is only available on mips and x86??
> > 
> 
> Looks to be if there are other folks in line for the ticket lock.
> 
> Arguably this should probably be something silly in spinlock.h like,
> 
> #ifndef __raw_spin_is_contended
> # define spin_is_contended (0)
> #else
> # define spin_is_contended(x) __raw_spin_is_contended((x))
> #endif
> 
> or something since in theory anybody has a chance of acquiring it next
> (well, lies because caches bias, but there's no waiters at least.)
> 
> Either that, or we just define it to be spin_is_locked if the assumption
> is that the lock will continue to be locked...

Ok, I thought this would be defined to something simple for the
non-ticket users, should I just not be using this call?  

-chris

Re: Build failure with latest -git: btrfs on ppc64

[Kyle McMartin]

On Mon, Feb 09, 2009 at 09:57:26AM -0500, Chris Mason wrote:
> 
> Ok, I thought this would be defined to something simple for the
> non-ticket users, should I just not be using this call?  
> 

I think the use is fine, since it's really a judgement call based on
whether there's waiters for the ticket. I need to take a closer look
at where this is used to say...

It's inevitable for you to hit things like this though, as far as I know
you're the first user of this lock interface. :)

regards, Kyle

Re: Build failure with latest -git: btrfs on ppc64

[Chris Mason]

On Mon, 2009-02-09 at 10:57 -0500, Kyle McMartin wrote:
> On Mon, Feb 09, 2009 at 09:57:26AM -0500, Chris Mason wrote:
> > 
> > Ok, I thought this would be defined to something simple for the
> > non-ticket users, should I just not be using this call?  
> > 
> 
> I think the use is fine, since it's really a judgement call based on
> whether there's waiters for the ticket. I need to take a closer look
> at where this is used to say...
> 

The Btrfs use goes something like this:

I'm about to wait for the disk.  If there are other procs waiting for
the locks I'm holding, I'll drop my locks and do a somewhat expensive
btree search over again later so the waiters can make progress.

If nobody is waiting, I'll do my read with my locks held so that I don't
have to search again.

> It's inevitable for you to hit things like this though, as far as I know
> you're the first user of this lock interface. :)

;)

-chris

Re: Build failure with latest -git: btrfs on ppc64

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Kyle McMartin wrote:
> On Sat, Feb 07, 2009 at 03:50:39PM -0500, Chuck Ebbert wrote:
>> fs/btrfs/locking.c: In function 'btrfs_path_lock_waiting':
>> fs/btrfs/locking.c:254: error: implicit declaration of function '__raw_spin_is_contended'
>>
>> Looks like spin_is_contended() is only available on mips and x86??
>>
> 
> Looks to be if there are other folks in line for the ticket lock.
> 
> Arguably this should probably be something silly in spinlock.h like,
> 
> #ifndef __raw_spin_is_contended
> # define spin_is_contended (0)
> #else
> # define spin_is_contended(x) __raw_spin_is_contended((x))
> #endif
> 
> or something since in theory anybody has a chance of acquiring it next
> (well, lies because caches bias, but there's no waiters at least.)
> 
> Either that, or we just define it to be spin_is_locked if the assumption
> is that the lock will continue to be locked...

I fixed it in my tree using the following patch.

From: Jeff Mahoney <jeffm@suse.com>
Subject: [PATCH] spin_is_contended Kconfig fixes

 include/linux/spinlock.h contains the following chunk:
 #ifdef CONFIG_GENERIC_LOCKBREAK
 #define spin_is_contended(lock) ((lock)->break_lock)
 #else
 #define spin_is_contended(lock) __raw_spin_is_contended(&(lock)->raw_lock)
 #endif

 __raw_spin_is_contended() is only implemented on mips and x86.

 Prior to a recent commit against btrfs, spin_needbreak was the only
 user of spin_is_contended, and it returns 0 when !CONFIG_PREEMPT. The
 architecture Kconfigs reflect that, depending on PREEMPT to enable
 GENERIC_LOCKBREAK. With the btrfs changes, this is causing build
 failures on anything !x86 (and mips, but I don't build on mips).

 Now that there is another user of spin_is_contended, the PREEMPT
 dependency is obsolete. This removes the dependency.

 This patch also adds GENERIC_LOCKBREAK entries to architectures that
 didn't have it and offer SMP support.

 The removal of the SPARC64 dependency isn't an oversight - SMP is
 supported for 32-bit sparc, so it would run into the same problem.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
- ---
 arch/alpha/Kconfig    |    5 +++++
 arch/arm/Kconfig      |    2 +-
 arch/blackfin/Kconfig |    5 +++++
 arch/ia64/Kconfig     |    2 +-
 arch/m32r/Kconfig     |    2 +-
 arch/parisc/Kconfig   |    2 +-
 arch/powerpc/Kconfig  |    2 +-
 arch/s390/Kconfig     |    2 +-
 arch/sh/Kconfig       |    7 ++++++-
 arch/sparc/Kconfig    |    2 +-
 10 files changed, 23 insertions(+), 8 deletions(-)

- --- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -25,6 +25,11 @@ config MMU
 config RWSEM_GENERIC_SPINLOCK
 	bool
 
+config GENERIC_LOCKBREAK
+        bool
+        default y
+        depends on SMP
+
 config RWSEM_XCHGADD_ALGORITHM
 	bool
 	default y
- --- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -115,7 +115,7 @@ config GENERIC_IRQ_PROBE
 config GENERIC_LOCKBREAK
 	bool
 	default y
- -	depends on SMP && PREEMPT
+	depends on SMP
 
 config RWSEM_GENERIC_SPINLOCK
 	bool
- --- a/arch/blackfin/Kconfig
+++ b/arch/blackfin/Kconfig
@@ -21,6 +21,11 @@ config RWSEM_XCHGADD_ALGORITHM
 	bool
 	default n
 
+config GENERIC_LOCKBREAK
+        bool
+        default y
+        depends on SMP
+
 config BLACKFIN
 	bool
 	default y
- --- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -58,7 +58,7 @@ config IOMMU_HELPER
 config GENERIC_LOCKBREAK
 	bool
 	default y
- -	depends on SMP && PREEMPT
+	depends on SMP
 
 config RWSEM_XCHGADD_ALGORITHM
 	bool
- --- a/arch/m32r/Kconfig
+++ b/arch/m32r/Kconfig
@@ -243,7 +243,7 @@ config IRAM_SIZE
 config GENERIC_LOCKBREAK
 	bool
 	default y
- -	depends on SMP && PREEMPT
+	depends on SMP
 
 config RWSEM_GENERIC_SPINLOCK
 	bool
- --- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -27,7 +27,7 @@ config STACK_GROWSUP
 config GENERIC_LOCKBREAK
 	bool
 	default y
- -	depends on SMP && PREEMPT
+	depends on SMP
 
 config RWSEM_GENERIC_SPINLOCK
 	def_bool y
- --- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -75,7 +75,7 @@ config RWSEM_XCHGADD_ALGORITHM
 config GENERIC_LOCKBREAK
 	bool
 	default y
- -	depends on SMP && PREEMPT
+	depends on SMP
 
 config ARCH_HAS_ILOG2_U32
 	bool
- --- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -63,7 +63,7 @@ config NO_DMA
 config GENERIC_LOCKBREAK
 	bool
 	default y
- -	depends on SMP && PREEMPT
+	depends on SMP
 
 config PGSTE
 	bool
- --- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -43,6 +43,11 @@ config RWSEM_GENERIC_SPINLOCK
 config RWSEM_XCHGADD_ALGORITHM
 	bool
 
+config GENERIC_LOCKBREAK
+        bool
+        default y
+        depends on SMP
+
 config GENERIC_BUG
 	def_bool y
 	depends on BUG && SUPERH32
@@ -82,7 +87,7 @@ config GENERIC_CLOCKEVENTS_BROADCAST
 
 config GENERIC_LOCKBREAK
 	def_bool y
- -	depends on SMP && PREEMPT
+	depends on SMP
 
 config SYS_SUPPORTS_PM
 	bool
- --- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -298,7 +298,7 @@ config US3_MC
 config GENERIC_LOCKBREAK
 	bool
 	default y
- -	depends on SPARC64 && SMP && PREEMPT
+	depends on SMP
 
 choice
 	prompt "SPARC64 Huge TLB Page Size"


- -- 
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkmQonwACgkQLPWxlyuTD7JPyQCghVJ0vgGlCxAbKZQMPaJxrIYZ
ePYAoIbujOBBnc0wqgn2bG+5KB11g8HL
=7r7Q
-----END PGP SIGNATURE-----

Re: Build failure with latest -git: btrfs on ppc64

[Chris Mason]

On Sat, 2009-02-07 at 15:50 -0500, Chuck Ebbert wrote:
> fs/btrfs/locking.c: In function 'btrfs_path_lock_waiting':
> fs/btrfs/locking.c:254: error: implicit declaration of function '__raw_spin_is_contended'
> 
> Looks like spin_is_contended() is only available on mips and x86??

Btrfs was using spin_is_contended to decide if it should drop locks
before doing an expensive operation during the core btree search.  After
a few latency tests here, I think it makes more sense to always drop
them instead.

Linus, if you pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-unstable.git

You'll get the build fix for btrfs on arches without ticket spinlocks 

 fs/btrfs/ctree.c   |    3 +--
 fs/btrfs/locking.c |   22 ----------------------
 fs/btrfs/locking.h |    2 --
 3 files changed, 1 insertion(+), 26 deletions(-)

-chris