Re: [PATCH] net: dev_getfirstbyhwtype() optimization

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Fri, Mar 19, 2010 at 06:14:02AM +0100, Eric Dumazet wrote:
> Le jeudi 18 mars 2010 à 19:32 -0700, Paul E. McKenney a écrit :
> > On Thu, Mar 18, 2010 at 10:27:25PM +0100, Eric Dumazet wrote:
> > > Use RCU to avoid RTNL use in dev_getfirstbyhwtype()
> > > 
> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > > ---
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 17b1686..0f2e9fc 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -772,14 +772,17 @@ EXPORT_SYMBOL(__dev_getfirstbyhwtype);
> > > 
> > >  struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type)
> > >  {
> > > -	struct net_device *dev;
> > > +	struct net_device *dev, *ret = NULL;
> > > 
> > > -	rtnl_lock();
> > > -	dev = __dev_getfirstbyhwtype(net, type);
> > > -	if (dev)
> > > -		dev_hold(dev);
> > > -	rtnl_unlock();
> > > -	return dev;
> > > +	rcu_read_lock();
> > > +	for_each_netdev_rcu(net, dev)
> > > +		if (dev->type == type) {
> > > +			dev_hold(dev);
> > > +			ret = dev;
> > > +			break;
> > > +		}
> > > +	rcu_read_unlock();
> > > +	return ret;
> > 
> > Looks good, but I don't understand how it helps to introduce the
> > local variable "ret".
> > 
> 
> Thanks for reviewing Paul !
> 
> Not only it helps, its necessary :)
> 
> for_each_netdev_rcu(net, dev) {
> 	if (cond) {
> 	   dev_hold(dev);
> 	   break;
> 	}
> }
> makes no guarantee dev is NULL if we hit the list end :
> 
> 
> /**
>  * list_for_each_entry_rcu      -       iterate over rcu list of given type
>  * @pos:        the type * to use as a loop cursor.
>  * @head:       the head for your list.
>  * @member:     the name of the list_struct within the struct.
>  *
>  * This list-traversal primitive may safely run concurrently with
>  * the _rcu list-mutation primitives such as list_add_rcu()
>  * as long as the traversal is guarded by rcu_read_lock().
>  */
> #define list_for_each_entry_rcu(pos, head, member) \
>         for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
>                 prefetch(pos->member.next), &pos->member != (head); \
>                 pos = list_entry_rcu(pos->member.next, typeof(*pos), member))

Right!  Got it, thank you!

							Thanx, Paul