Re: [PATCH net-next V3] net: dynamic ingress_queue allocation

[Jarek Poplawski <jarkao2@gmail.com>]

On Mon, Oct 04, 2010 at 10:42:09AM +0200, Eric Dumazet wrote:
> Le dimanche 03 octobre 2010 ?? 11:42 +0200, Jarek Poplawski a écrit :
> 
> > I'd consider rcu_dereference_rtnl(). Btw, technically qdisc_lookup()
> > doesn't require rtnl, and there was time it was used without it
> > (on xmit path).
> 
> 
> Hmm, for me, rcu_dereference_rtnl() is a bit lazy.
> 
> Either we are a reader and should use rcu_dereference(), or a writer and
> RTNL should be held.
> 
> Mixing two conditions in a "super macro" is a workaround that we used to
> promptly shutup some lockdep splats. Real fix would be to use strict
> lockdep conditions, because this better documents the code and the
> locking invariants.
> 
> BTW, rtnl_dereference() should be changed to use
> rcu_dereference_protected() instead of rcu_dereference_check() :
> If RTBL is held, there is no need to force a barrier.

Actually, I'm one of those (convinced by Patrick btw), who consider
rcu_dereference() on the clear update side as misleading, so I'm not
rtnl_dereference() fan with or without changes.

> 
> 
> > I think you should also add a comment here why this rcu is used, and
> > that it changes only once in dev's liftime.
> > 
> 
> This comment was needed in the previous version of the patch, with the
> smb_wmb() barrier. Now I switched to regular rcu use, nothing prevents
> us to change dev->ingress_queue in flight. Of course there is no current
> interest doing so.

Right, but then at least in qdisc_lookup():

if (dev_ingress_queue(dev))
	q = qdisc_match_from_root(dev_ingress_queue(dev), handle);

you should use a variable instead of the second dereference (rtnl isn't
mandatory here).

Jarek P.