Daniel Phillips wrote:
> Any idea how one might implement NEVER_SLEEPS()? Maybe as:
Why would you want that? AFAICS there are two kinds of "never
sleeping" functions: 1. those that don't sleep but don't care
about it and 2. those that must not sleep because a lock is
held.
For 1. no point marking it because it might change without
being a bug. You also don't want to mark every function
in the kernel SLEEPS() or NEVER_SLEEPS().
For 2. we already have MUST_HOLD(foo) or similar, which implicitly
means it can never sleep. The same is true for functions
with spinlocks or preempt_disable around their body.
Arnd <><
On Friday 12 July 2002 21:24, Arnd Bergmann wrote:
> Daniel Phillips wrote:
>
> > Any idea how one might implement NEVER_SLEEPS()? Maybe as:
>
> Why would you want that? AFAICS there are two kinds of "never
> sleeping" functions: 1. those that don't sleep but don't care
> about it and 2. those that must not sleep because a lock is
> held.
>
> For 1. no point marking it because it might change without
> being a bug. You also don't want to mark every function
> in the kernel SLEEPS() or NEVER_SLEEPS().
>
> For 2. we already have MUST_HOLD(foo) or similar, which implicitly
> means it can never sleep. The same is true for functions
> with spinlocks or preempt_disable around their body.
Thanks for that.
So far, only the MUST_HOLD style of executable locking documentation has
really survived scrutiny. It needs some variants: MUST_HOLD_READ,
MUST_HOLD_WRITE, MUST_HOLD_SEM, MUST_HOLD_READ_SEM and MUST_HOLD_WRITE_SEM,
or names to that effect.
--
Daniel
On Fri, Jul 12, 2002 at 07:42:09PM +0200, Daniel Phillips wrote:
> So far, only the MUST_HOLD style of executable locking documentation has
> really survived scrutiny. It needs some variants: MUST_HOLD_READ,
> MUST_HOLD_WRITE, MUST_HOLD_SEM, MUST_HOLD_READ_SEM and MUST_HOLD_WRITE_SEM,
> or names to that effect.
I'm not quite sure where to put the semaphore versions of the MUST_*
macros, I guess they'd have to go in each of the arch-specific header
files? Anyway, I've got spinlock and rwlock versions of them below,
maybe they're useful enough to go in as a start? I only coded the
ia64 version of rwlock_is_*_locked since it was easy--the i386
versions were a little intimidating...
I thought Oliver's suggestion for tracking the order of spinlock
acquisition was good, hopefully someone will take a stab at it along
with Dave's FUNCTION_SLEEPS() implementation.
Jesse
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/fs/inode.c linux-2.5.25-spinassert/fs/inode.c
--- linux-2.5.25/fs/inode.c Fri Jul 5 16:42:38 2002
+++ linux-2.5.25-spinassert/fs/inode.c Tue Jul 16 19:04:01 2002
@@ -183,6 +183,8 @@
*/
void __iget(struct inode * inode)
{
+ MUST_HOLD_SPIN(&inode_lock);
+
if (atomic_read(&inode->i_count)) {
atomic_inc(&inode->i_count);
return;
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/include/asm-ia64/spinlock.h linux-2.5.25-spinassert/include/asm-ia64/spinlock.h
--- linux-2.5.25/include/asm-ia64/spinlock.h Fri Jul 5 16:42:03 2002
+++ linux-2.5.25-spinassert/include/asm-ia64/spinlock.h Tue Jul 16 18:31:53 2002
@@ -109,6 +109,8 @@
#define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }
#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+#define rwlock_is_read_locked(x) ((x)->read_counter != 0 || (x)->write_lock != 0)
+#define rwlock_is_write_locked(x) ((x)->write_lock != 0)
#define _raw_read_lock(rw) \
do { \
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.25/include/linux/spinlock.h linux-2.5.25-spinassert/include/linux/spinlock.h
--- linux-2.5.25/include/linux/spinlock.h Fri Jul 5 16:42:24 2002
+++ linux-2.5.25-spinassert/include/linux/spinlock.h Tue Jul 16 18:58:40 2002
@@ -116,7 +116,21 @@
#define _raw_write_lock(lock) (void)(lock) /* Not "unused variable". */
#define _raw_write_unlock(lock) do { } while(0)
-#endif /* !SMP */
+#endif /* !CONFIG_SMP */
+
+/*
+ * Simple lock assertions for debugging and documenting where locks need
+ * to be locked/unlocked.
+ */
+#if defined(CONFIG_DEBUG_SPINLOCK) && defined(CONFIG_SMP)
+#define MUST_HOLD_SPIN(lock) BUG_ON(!spin_is_locked(lock))
+#define MUST_HOLD_READ(lock) BUG_ON(!rwlock_is_read_locked(lock))
+#define MUST_HOLD_WRITE(lock) BUG_ON(!rwlock_is_write_locked(lock))
+#else
+#define MUST_HOLD_SPIN(lock) do { } while(0)
+#define MUST_HOLD_READ(lock) do { } while(0)
+#define MUST_HOLD_WRITE(lock) do { } while(0)
+#endif /* CONFIG_DEBUG_SPINLOCK && CONFIG_SMP */
#ifdef CONFIG_PREEMPT
On Wednesday 17 July 2002 04:22, Jesse Barnes wrote:
> On Fri, Jul 12, 2002 at 07:42:09PM +0200, Daniel Phillips wrote:
> > So far, only the MUST_HOLD style of executable locking documentation has
> > really survived scrutiny. It needs some variants: MUST_HOLD_READ,
> > MUST_HOLD_WRITE, MUST_HOLD_SEM, MUST_HOLD_READ_SEM and MUST_HOLD_WRITE_SEM,
> > or names to that effect.
>
> I'm not quite sure where to put the semaphore versions of the MUST_*
> macros, I guess they'd have to go in each of the arch-specific header
> files?
You could create linux/semaphore.h which includes asm/semaphore.h, making
the whole arrangement more similar to spinlocks. That would be the manly
thing to do, however, manliness not necessarily being the fashion at the
moment, putting them in the arch-specific headers seems like the route of
least resistance. One day, a prince on a white horse will come along and
clean up all the header files...
> Anyway, I've got spinlock and rwlock versions of them below,
> maybe they're useful enough to go in as a start? I only coded the
> ia64 version of rwlock_is_*_locked since it was easy--the i386
> versions were a little intimidating...
>
> I thought Oliver's suggestion for tracking the order of spinlock
> acquisition was good, hopefully someone will take a stab at it along
> with Dave's FUNCTION_SLEEPS() implementation.
Indeed, it's a nice realization of Dave's idea, very clever.
On the minor niggle side, I think "MUST_HOLD" scans better than
"MUST_HOLD_SPIN", since the former is closer to the way we say it when
we're talking amongst ourselves.
--
Daniel
On Wednesday 17 July 2002 04:22, Jesse Barnes wrote:
> files? Anyway, I've got spinlock and rwlock versions of them below,
> maybe they're useful enough to go in as a start? I only coded the
> ia64 version of rwlock_is_*_locked since it was easy--the i386
> versions were a little intimidating...
>
> I thought Oliver's suggestion for tracking the order of spinlock
> acquisition was good, hopefully someone will take a stab at it along
> with Dave's FUNCTION_SLEEPS() implementation.
I suppose you can simplify your interface when the code tracking the lock
holder (i.e. the address of the lock call) is there:
#define MUST_HOLD(lock) BUG_ON(!(lock)->holder)
is independent of whether lock is a spinlock or an rw_lock, so you
don't need MUST_HOLD_READ anymore. I'd even go as far as dropping
MUST_HOLD_WRITE as well, since it helps only in the corner case
where the lock is held but only for reading.
Arnd <><
On Wed, Jul 17, 2002 at 08:34:05AM +0200, Daniel Phillips wrote:
> You could create linux/semaphore.h which includes asm/semaphore.h, making
> the whole arrangement more similar to spinlocks. That would be the manly
> thing to do, however, manliness not necessarily being the fashion at the
> moment, putting them in the arch-specific headers seems like the route of
> least resistance. One day, a prince on a white horse will come along and
> clean up all the header files...
Well, I'll at least take a stab at it, but I won't have time until
next week. Here's the current version of the macros against 2.5.26
though, in case someone wants to add support for architectures other
than ia64.
> > I thought Oliver's suggestion for tracking the order of spinlock
> > acquisition was good, hopefully someone will take a stab at it along
> > with Dave's FUNCTION_SLEEPS() implementation.
It doesn't look like it would be too hard to do, but seems like it
should be a seperate patch (maybe with more header file tweaking).
> On the minor niggle side, I think "MUST_HOLD" scans better than
> "MUST_HOLD_SPIN", since the former is closer to the way we say it when
> we're talking amongst ourselves.
Sure thing. I fixed it up in this version.
Thanks,
Jesse
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.26/fs/inode.c linux-2.5.26-lockassert/fs/inode.c
--- linux-2.5.26/fs/inode.c Tue Jul 16 16:49:38 2002
+++ linux-2.5.26-lockassert/fs/inode.c Thu Jul 18 10:21:13 2002
@@ -183,6 +183,8 @@
*/
void __iget(struct inode * inode)
{
+ MUST_HOLD(&inode_lock);
+
if (atomic_read(&inode->i_count)) {
atomic_inc(&inode->i_count);
return;
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.26/include/asm-ia64/spinlock.h linux-2.5.26-lockassert/include/asm-ia64/spinlock.h
--- linux-2.5.26/include/asm-ia64/spinlock.h Tue Jul 16 16:49:25 2002
+++ linux-2.5.26-lockassert/include/asm-ia64/spinlock.h Thu Jul 18 16:30:49 2002
@@ -109,6 +109,7 @@
#define RW_LOCK_UNLOCKED (rwlock_t) { 0, 0 }
#define rwlock_init(x) do { *(x) = RW_LOCK_UNLOCKED; } while(0)
+#define rwlock_is_locked(x) ((x)->read_counter != 0 || (x)->write_lock != 0)
#define _raw_read_lock(rw) \
do { \
diff -Naur -X /home/jbarnes/dontdiff linux-2.5.26/include/linux/spinlock.h linux-2.5.26-lockassert/include/linux/spinlock.h
--- linux-2.5.26/include/linux/spinlock.h Tue Jul 16 16:49:33 2002
+++ linux-2.5.26-lockassert/include/linux/spinlock.h Thu Jul 18 16:31:13 2002
@@ -116,7 +116,19 @@
#define _raw_write_lock(lock) (void)(lock) /* Not "unused variable". */
#define _raw_write_unlock(lock) do { } while(0)
-#endif /* !SMP */
+#endif /* !CONFIG_SMP */
+
+/*
+ * Simple lock assertions for debugging and documenting where locks need
+ * to be locked/unlocked.
+ */
+#if defined(CONFIG_DEBUG_SPINLOCK) && defined(CONFIG_SMP)
+#define MUST_HOLD(lock) BUG_ON(!spin_is_locked(lock))
+#define MUST_HOLD_RW(lock) BUG_ON(!rwlock_is_locked(lock))
+#else
+#define MUST_HOLD(lock) do { } while(0)
+#define MUST_HOLD_RW(lock) do { } while(0)
+#endif /* CONFIG_DEBUG_SPINLOCK && CONFIG_SMP */
#ifdef CONFIG_PREEMPT