Messages in this thread Patch in this message | | | Subject | Re: [RFC][PATCH 2/7] lockdep: Remove redundant read checks | From | Steven Rostedt <> | Date | Mon, 18 Apr 2011 10:28:15 -0400 |
| |
On Sun, 2011-04-17 at 11:45 +0200, Peter Zijlstra wrote: > plain text document attachment > (gautham_r_shenoy-lockdep-remove_redundant_read_checks_.patch) > From: Gautham R Shenoy <ego@in.ibm.com> > > Do various simplifications: > > 1) In kernel/lockdep.c::validate_chain(): > > ret = check_deadlock(curr, hlock, lock, hlock->read); > > ret == 2 only if hlock->read == 2. > > Hence: > > if (ret == 2) > hlock->read = 2; > > is redundant and can be removed.
Is this really true? From check_deadlock():
/* * Allow read-after-read recursion of the same * lock class (i.e. read_lock(lock)+read_lock(lock)): */ if ((read == 2) && prev->read) return 2;
/* * We're holding the nest_lock, which serializes this lock's * nesting behaviour. */ if (nest) return 2;
We return '2' also when we nest.
> > 2) In kernel/lockdep.c::check_prevs_add(curr, next): > > if (hlock->read != 2) > check_prev_add(curr, hlock, next, distance); > > Thus, check_prev_add is called only when hlock->read != 2. > > >From the conclusions of 2): > > kernel/lockdep.c::check_prev_add(curr, prev, next, distance) gets called > iff prev->read != 2. > > Hence, in kernel/lockdep.c::check_prev_add(curr, prev, next, distance): > > if (prev->read == 2) > return 1; > > is redunant and can be removed.
I agree with this one, but perhaps a comment should be added in its place.
-- Steve
> > Signed-off-by: Gautham R Shenoy <ego@in.ibm.com> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > --- > kernel/lockdep.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > Index: tip/kernel/lockdep.c > =================================================================== > --- tip.orig/kernel/lockdep.c > +++ tip/kernel/lockdep.c > @@ -1676,7 +1676,7 @@ check_prev_add(struct task_struct *curr, > * write-lock never takes any other locks, then the reads are > * equivalent to a NOP. > */ > - if (next->read == 2 || prev->read == 2) > + if (next->read == 2) > return 1; > /* > * Is the <prev> -> <next> dependency already present? > @@ -1940,13 +1940,6 @@ static int validate_chain(struct task_st > if (!ret) > return 0; > /* > - * Mark recursive read, as we jump over it when > - * building dependencies (just like we jump over > - * trylock entries): > - */ > - if (ret == 2) > - hlock->read = 2; > - /* > * Add dependency only if this lock is not the head > * of the chain, and if it's not a secondary read-lock: > */ >
| |