On Mon, Jul 20, 2009 at 10:49:07AM +0200, Peter Zijlstra wrote: > On Mon, 2009-07-20 at 04:12 -0400, Frederic Weisbecker wrote: > > > From: Frederic Weisbecker <fweisbec@xxxxxxxxx> > > Subject: [PATCH] sched: Check if the spinlock is locked in cond_resched_lock() > > > > Some uses of cond_resched_lock() might involve an > > unlocked spinlock, resulting in spurious sleep in > > atomic warnings. > > Check whether the spinlock is actually locked and > > take that into account in the might_sleep() check. > > > > Reported-by: Li Zefan <lizf@xxxxxxxxxxxxxx> > > Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx> > > --- > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index cb070dc..2789658 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -2294,9 +2294,10 @@ extern int _cond_resched(void); > > > > extern int __cond_resched_lock(spinlock_t *lock); > > > > -#define cond_resched_lock(lock) ({ \ > > - __might_sleep(__FILE__, __LINE__, PREEMPT_OFFSET); \ > > - __cond_resched_lock(lock); \ > > +#define cond_resched_lock(lock) ({ \ > > + __might_sleep(__FILE__, __LINE__, spin_is_locked(lock) ? \ > > + PREEMPT_OFFSET : 0); \ > > + __cond_resched_lock(lock); \ > > }) > > > > extern int __cond_resched_softirq(void); > > > No, this looks utterly broken.. who is to say it doesn't get unlocked > right after that spin_is_locked() check? Oh, indeed, that was silly... > > cond_resched_lock() callers must hold the lock they use it on, not doing > so is broken. > > So I would suggest something like the below instead: > > (utterly untested) Looks better anyway. Thanks, Frederic. > > Almost-signed-off-by: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> > --- > include/linux/lockdep.h | 8 ++++++++ > kernel/lockdep.c | 33 +++++++++++++++++++++++++++++++++ > kernel/sched.c | 2 ++ > 3 files changed, 43 insertions(+), 0 deletions(-) > > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h > index 12aabfc..920df42 100644 > --- a/include/linux/lockdep.h > +++ b/include/linux/lockdep.h > @@ -296,6 +296,10 @@ extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass, > extern void lock_release(struct lockdep_map *lock, int nested, > unsigned long ip); > > +#define lockdep_is_held(lock) lock_is_held(&(lock)->dep_map) > + > +extern int lock_is_held(struct lockdep_map *lock); > + > extern void lock_set_class(struct lockdep_map *lock, const char *name, > struct lock_class_key *key, unsigned int subclass, > unsigned long ip); > @@ -314,6 +318,8 @@ extern void lockdep_trace_alloc(gfp_t mask); > > #define lockdep_depth(tsk) (debug_locks ? (tsk)->lockdep_depth : 0) > > +#define lockdep_assert_held(l) WARN_ON(!lockdep_is_held(l)) > + > #else /* !LOCKDEP */ > > static inline void lockdep_off(void) > @@ -358,6 +364,8 @@ struct lock_class_key { }; > > #define lockdep_depth(tsk) (0) > > +#define lockdep_assert_held(l) do { } while (0) > + > #endif /* !LOCKDEP */ > > #ifdef CONFIG_LOCK_STAT > diff --git a/kernel/lockdep.c b/kernel/lockdep.c > index 3718a98..fd626ea 100644 > --- a/kernel/lockdep.c > +++ b/kernel/lockdep.c > @@ -3044,6 +3044,19 @@ __lock_release(struct lockdep_map *lock, int nested, unsigned long ip) > check_chain_key(curr); > } > > +static int __lock_is_held(struct lockdep_map *lock) > +{ > + struct task_struct *curr = current; > + int i; > + > + for (i = 0; i < curr->lockdep_depth; i++) { > + if (curr->held_locks[i].instance == lock) > + return 1; > + } > + > + return 0; > +} > + > /* > * Check whether we follow the irq-flags state precisely: > */ > @@ -3145,6 +3158,26 @@ void lock_release(struct lockdep_map *lock, int nested, > } > EXPORT_SYMBOL_GPL(lock_release); > > +int lock_is_held(struct lockdep_map *lock) > +{ > + unsigned long flags; > + int ret = 0; > + > + if (unlikely(current->lockdep_recursion)) > + return; > + > + raw_local_irq_save(flags); > + check_flags(flags); > + > + current->lockdep_recursion = 1; > + ret = __lock_is_held(lock); > + current->lockdep_recursion = 0; > + raw_local_irq_restore(flags); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(lock_is_held); > + > void lockdep_set_current_reclaim_state(gfp_t gfp_mask) > { > current->lockdep_reclaim_gfp = gfp_mask; > diff --git a/kernel/sched.c b/kernel/sched.c > index 56fb225..6255f82 100644 > --- a/kernel/sched.c > +++ b/kernel/sched.c > @@ -6638,6 +6638,8 @@ int __cond_resched_lock(spinlock_t *lock) > int resched = should_resched(); > int ret = 0; > > + lockdep_assert_held(lock); > + > if (spin_needbreak(lock) || resched) { > spin_unlock(lock); > if (resched) > > -- To unsubscribe from this list: send the line "unsubscribe linux-tip-commits" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html