Re: [RFC][PATCH 2/7] lockdep: Remove redundant read checks

From: Steven Rostedt
Date: Mon Apr 18 2011 - 10:28:27 EST


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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> ---
> 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:
> */
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/