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

[Steven Rostedt <rostedt@goodmis.org>]

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:
>  		 */
>