Re: [PATCH] net: dev_getfirstbyhwtype() optimization

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

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))