Messages in this thread | | | Date | Thu, 9 Jan 2014 18:08:23 +0100 | From | Peter Zijlstra <> | Subject | Re: [RFC][PATCH] lockdep: Introduce wait-type checks |
| |
On Thu, Jan 09, 2014 at 05:31:20PM +0100, Oleg Nesterov wrote: > This is really minor, but it seems you can simplify it a little bit. > We do not really need curr_inner, the main loop can do > > for (...) { > ... > > if (prev_inner && prev_inner < next_outer) > return print_lock_invalid_wait_context(...); > } > > return 0;
Indeed, that might work.. My brain still finds it easier to understand the min() version though.
> Off-topic question... I can't understand the "int check" argument of > lock_acquire(). First of all, __lock_acquire() does > > if (!prove_locking) > check = 1; > > Doesn't this mean lock_acquire_*() do not depend on CONFIG_PROVE_LOCKING? > IOW, can't we do > > --- x/include/linux/lockdep.h > +++ x/include/linux/lockdep.h > @@ -479,15 +479,9 @@ static inline void print_irqtrace_events > * on the per lock-class debug mode: > */ > > -#ifdef CONFIG_PROVE_LOCKING > - #define lock_acquire_exclusive(l, s, t, n, i) lock_acquire(l, s, t, 0, 2, n, i) > - #define lock_acquire_shared(l, s, t, n, i) lock_acquire(l, s, t, 1, 2, n, i) > - #define lock_acquire_shared_recursive(l, s, t, n, i) lock_acquire(l, s, t, 2, 2, n, i) > -#else > - #define lock_acquire_exclusive(l, s, t, n, i) lock_acquire(l, s, t, 0, 1, n, i) > - #define lock_acquire_shared(l, s, t, n, i) lock_acquire(l, s, t, 1, 1, n, i) > - #define lock_acquire_shared_recursive(l, s, t, n, i) lock_acquire(l, s, t, 2, 1, n, i) > -#endif > +#define lock_acquire_exclusive(l, s, t, n, i) lock_acquire(l, s, t, 0, 2, n, i) > +#define lock_acquire_shared(l, s, t, n, i) lock_acquire(l, s, t, 1, 2, n, i) > +#define lock_acquire_shared_recursive(l, s, t, n, i) lock_acquire(l, s, t, 2, 2, n, i)
I suppose we could; note however that the if (!prove_locking) logic was added later.
> But what I really can't understans is what "check == 0" means? It > seems that in fact it can be 1 or 2? Or, iow, "check == 0" is actually > equivalent to "check == 1" ?
Hmm indeed, the comment in lockdep.h says 0 means no checks at all, but the code doesn't actually appear to work like that. I'm not sure it ever did or not, I'd have to go dig through history.
That said, given the current state it certainly looks like we can remove the check argument.
Ingo?
| |