Re: [PATCH 00/10] __rcu annotations, first draft

[Ingo Molnar <mingo@elte.hu>]

* Arnd Bergmann <arnd@arndb.de> wrote:

> On Tuesday 23 February 2010, Paul E. McKenney wrote:
> > > I've just started an experimental implementation and got stuck at list rcu.
> > > The two to deal with it that I can see are
> > > - ignore list-rcu for now, and make all include/linux/rculist.h __force the
> > >   problem to be ignored.
> > > - introduce a new struct rcu_list_head that needs to be used for list rcu.
> > >
> > > A nicer option might be if sparse would let you write
> > > 'struct list_head __rcu head' and interpret that as having the pointers
> > > inside it annotated as __rcu.
> >
> > Only the "next" pointer, not the "prev" pointer, but yes.
> >
> > Perhaps it would be best to see if the sparse guys are willing to take
> > this on?
> 
> I've implemented the second option now, and this seems to work alright.
> In order to test this out, I've annotated all rcu users in the kernel
> directory. One observation is that many members of task_struct are
> using RCU inconsistently. I'm not sure whether these should be considered
> bugs or false positives though. If we get a lot of false positives or
> people are generally unhappy about the annotations, they are not worth
> it, but if we find a few actual bugs, I guess it would be good to
> have this.
> 
> Examples of pointers that I did not annotate because most of the
> places accessing them do not use RCU are task->sighand, task->nsproxy,
> and task->real_parent.
> 
> Sorry for hijacking the discussion on your patch, but I assume that
> the same people interested in the lockdep diagnostics are also
> interested in the static annotations for RCU.
> 
> 	Arnd
> 
> Arnd Bergmann (10):
>   rcu: define __rcu address space modifier for sparse
>   rcu: annotated list rcu code
>   cgroups: __rcu annotations
>   credentials: rcu annotation
>   perf_event: __rcu annotations
>   audit: __rcu annotations
>   module: __rcu annotations
>   pid: __rcu annotations
>   notifiers: __rcu annotations
>   scheduler: __rcu annotations
> 
>  include/linux/cgroup.h     |    6 +-
>  include/linux/compiler.h   |    2 +
>  include/linux/cred.h       |    4 +-
>  include/linux/fdtable.h    |    2 +-
>  include/linux/init_task.h  |    6 +-
>  include/linux/module.h     |    4 +-
>  include/linux/notifier.h   |   10 ++--
>  include/linux/perf_event.h |   10 ++--
>  include/linux/pid.h        |    9 ++-
>  include/linux/rculist.h    |  152 ++++++++++++++++++++++++++++++++------------
>  include/linux/rcupdate.h   |   46 ++++++++++++--
>  include/linux/sched.h      |   14 ++--
>  kernel/audit.c             |    4 +-
>  kernel/audit.h             |    6 +-
>  kernel/audit_tree.c        |   14 ++--
>  kernel/auditfilter.c       |   28 ++++----
>  kernel/auditsc.c           |    8 +-
>  kernel/cgroup.c            |   63 ++++++++++---------
>  kernel/cpuset.c            |    2 +-
>  kernel/cred.c              |   50 ++++++++-------
>  kernel/exit.c              |   12 ++--
>  kernel/fork.c              |    6 +-
>  kernel/module.c            |   20 ++++--
>  kernel/notifier.c          |   12 ++--
>  kernel/perf_event.c        |   52 ++++++++--------
>  kernel/pid.c               |    8 +-
>  kernel/sched.c             |   31 +++++----
>  27 files changed, 352 insertions(+), 229 deletions(-)

This looks very clean IMO.

By far the biggest remaining design aspects of RCU is that it's somewhat on 
the opaque side of APIs: it's not immediately obvious when a data structure 
(and subsequently, code using that date structure) is affected by RCU 
semantics.

This is good in terms of API utility: you _can_ use it with very little 
resistence on the way - you can combine RCU and non-RCU code with few 
syntactic constraints imposed.

That kind of flexibility inevitably has its downsides: with a quick guess i'd 
say more than half of the non-trivial RCU related usage bugs in various 
subsystem stemmed out of using non-RCU facilities on RCU data structures.

So the annotation both 'look' clean [which alone is a plus as people write new 
code] and they also might result in bugfixes ...

	Ingo