netdev - Re: [PATCH] net: dev_getfirstbyhwtype() optimization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 19 Mar 2010 06:14:02 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	paulmck@...ux.vnet.ibm.com
Cc:	David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: dev_getfirstbyhwtype() optimization

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@...il.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))



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html