Re: [PATCH tip/core/rcu 31/48] rcu: define __rcu address space modifier for sparse

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 04 May 2010 22:19:41 Paul E. McKenney wrote:
> This patch defines an __rcu annotation that permits sparse to check for
> correct use of RCU-protected pointers.  If a pointer that is annotated
> with __rcu is accessed directly (as opposed to via rcu_dereference(),
> rcu_assign_pointer(), or one of their variants), sparse can be made
> to complain.  To enable such complaints, use the new default-disabled
> CONFIG_SPARSE_RCU_POINTER kernel configuration option.  Please note that
> these sparse complaints are intended to be a debugging aid, -not- a
> code-style-enforcement mechanism.

To add more background, I was thinking that it might make sense to
always leave the address space attribute in place but to make part
part of the checking optional.

The idea would be that we always make sure that an __rcu annotated
pointer cannot be dereferenced or cast directly, while we would
only complain about non-annotated pointers being passed to rcu_dereference
and rcu_assign_pointer if CONFIG_SPARSE_RCU_POINTER is set.

Most of the work I had spent on my tree was about fixing all the
false positives from that, but more work would be needed to get
a clean build from it even with the modified CONFIG_SPARSE_RCU_POINTER
disabled. Since you managed to find the real bugs and fix them,
your series by itself is probably more useful than the full set
that I originally had.

> +/*
> + * Helper functions for rcu_dereference_check(), rcu_dereference_protected()
> + * and rcu_assign_pointer().  Some of these could be folded into their
> + * callers, but they are left separate in order to ease introduction of
> + * multiple flavors of pointers to match the multiple flavors of RCU
> + * (e.g., __rcu_bh, * __rcu_sched, and __srcu), should this make sense in
> + * the future.
> + */
> +#define __rcu_access_pointer(p, space) \
> +	({ \
> +		typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> +		(void) (((typeof (*p) space *)p) == p); \
> +		((typeof(*p) __force __kernel *)(_________p1)); \
> +	})

Do you have specific plans to add these (__rcu_bh etc) back in the future,
or do you just want to leave the options open?

Anyway, good to see that you found your way through my patches and got them
into shape. 

	Arnd