Re: [PATCH 2/3] percpu_rw_semaphore: add the lockdep annotations

[Andrew Morton <akpm@linux-foundation.org>]

On Sun, 18 Nov 2012 20:03:21 +0100
Oleg Nesterov <oleg@redhat.com> wrote:

> Add the lockdep annotations. Not only this can help to find the
> potential problems, we do not want the false warnings if, say,
> the task takes two different percpu_rw_semaphore's for reading.
> IOW, at least ->rw_sem should not use a single class.
> 
> This patch exposes this internal lock to lockdep so that it
> represents the whole percpu_rw_semaphore. This way we do not
> need to add another "fake" ->lockdep_map and lock_class_key.
> More importantly, this also makes the output from lockdep much
> more understandable if it finds the problem.
> 
> In short, with this patch from lockdep pov percpu_down_read()
> and percpu_up_read() acquire/release ->rw_sem for reading, this
> matches the actual semantics. This abuses __up_read() but I hope
> this is fine and in fact I'd like to have down_read_no_lockdep()
> as well, percpu_down_read_recursive_readers() will need it.
> 
> ...
>
> -extern int percpu_init_rwsem(struct percpu_rw_semaphore *);
> +extern int __percpu_init_rwsem(struct percpu_rw_semaphore *,
> +				const char *, struct lock_class_key *);
>  extern void percpu_free_rwsem(struct percpu_rw_semaphore *);
>  
> +#define percpu_init_rwsem(brw)	\

Should have been called percpu_rwsem_init().  The naming in this code does
seem to be rather inconsistent.  s/percpu_rw_semaphore/percpu_rwsem/g
would be a good start, then consistently use percpu_rwsem_foo where
practical.  But percpu_rwsem_down_read() doesn't sound practical :(


Is there much point in doing all these changes as five separate patches
(so far)?  Perhaps it should all blobbed into as little as one patch(es)?


You sent your uprobes changes to Ingo as a git pull, but I doubt if
Ingo's trees contain the percpu_rwsem_rw_semaphore changes.  What's
happening here?