Re: [PATCH net-next-2.6] bridge: add __rcu annotations

[Eric Dumazet <eric.dumazet@gmail.com>]

Le samedi 13 novembre 2010 à 10:13 -0800, Stephen Hemminger a écrit :
> On Sat, 13 Nov 2010 18:58:50 +0100
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > Le samedi 13 novembre 2010 à 09:35 -0800, Stephen Hemminger a écrit :
> > > On Sat, 13 Nov 2010 09:15:28 +0100
> > > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > 
> > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > > index 578debb..ffbd177 100644
> > > > --- a/include/linux/netdevice.h
> > > > +++ b/include/linux/netdevice.h
> > > > @@ -996,7 +996,10 @@ struct net_device {
> > > >  #endif
> > > >  
> > > >  	rx_handler_func_t	*rx_handler;
> > > > -	void			*rx_handler_data;
> > > > +	union {
> > > > +		void				*rx_handler_data;
> > > > +		struct net_bridge_port __rcu	*br_port_rcu;
> > > > +	};
> > > >  
> > > >  	struct netdev_queue __rcu *ingress_queue;
> > > 
> > > I don't like making the generic hook typed again.
> > > We don't do this for other callbacks, timers, workqueues, ...
> > > Why is it necessary for RCU notation.
> > > 
> > 
> > because rcu_dereference() needs the type for __CHECKER__/sparse checks
> > 
> > #define __rcu_dereference_check(p, c, space) \
> >         ({ \
> >                 typeof(*p) *_________p1 = (typeof(*p)*__force )ACCESS_ONCE(p); \
> >                 rcu_lockdep_assert(c); \
> >                 rcu_dereference_sparse(p, space); \
> >                 smp_read_barrier_depends(); \
> >                 ((typeof(*p) __force __kernel *)(_________p1)); \
> >         })
> > 
> > So using a "void *ptr" is not an option
> > 
> > Its also cleaner to use
> > 
> > rcu_dereference(dev->br_port_rcu)
> > 
> > instead of 
> > 
> > (struct net_bridge_port *)rcu_dereference(dev->rx_handler_data)
> 
> There must be a better way. What about use of that hook by macvlan and openvswitch?

macvlan and openvswitch (is it part of linux yet ???)

I honestly dont understand your point Stephen, maybe you could explain a
bit more what is the problem ?

I use a union, like many other ones in the kernel. This is the first
time I ear this is not good to add type safety.

You can use either one or other field at your convenience.

If you are talking about stacking hooks, that has nothing to do with
this (cleanup) rcu patch, but previous introduction of
rx_handler_data/rx_handler ?

Please run sparse on x86_64 machine and watch all the warnings in bridge
code. (with CONFIG_SPARSE_RCU_POINTER=y)

Me confused.