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

[Jarek Poplawski <jarkao2@gmail.com>]

On Sat, Oct 02, 2010 at 06:11:55PM +0200, Eric Dumazet wrote:
> Le samedi 02 octobre 2010 ?? 11:32 +0200, Jarek Poplawski a écrit :
> > On Fri, Oct 01, 2010 at 03:56:28PM +0200, Eric Dumazet wrote:
...
> > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > > index b802078..8635110 100644
> > > --- a/net/sched/sch_api.c
> > > +++ b/net/sched/sch_api.c
...
> > > @@ -690,6 +693,8 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
> > >  		    (new && new->flags & TCQ_F_INGRESS)) {
> > >  			num_q = 1;
> > >  			ingress = 1;
> > > +			if (!dev_ingress_queue(dev))
> > > +				return -ENOENT;
> > 
> > Is this test really needed here?
> 
> To avoid a NULL dereference some lines later.
> Do I have a guarantee its not NULL here ?

Do you have any scenario for NULL here? ;-)

Of course, it's your patch and responsibility, and I'll not guarantee,
but you could at least add a TODO comment, to check it later.

> > > @@ -1044,7 +1050,8 @@ replay:
> > >  					return -ENOENT;
> > >  				q = qdisc_leaf(p, clid);
> > >  			} else { /*ingress */
> > > -				q = dev->ingress_queue.qdisc_sleeping;
> > > +				if (dev_ingress_queue_create(dev))
> > > +					q = dev_ingress_queue(dev)->qdisc_sleeping;
> > 
> > I wonder if doing dev_ingress_queue_create() just before qdisc_create()
> > (and the test here) isn't more readable.
> 
> Sorry, I dont understand. I want to create ingress_queue only if user
> wants it. If we setup (egress) trafic shaping, no need to setup
> ingress_queue.

I mean doing both creates in one place:

> @@ -1123,11 +1130,14 @@ replay:
>  create_n_graft:
...
> +	if (clid == TC_H_INGRESS) {
 +		if (dev_ingress_queue_create(dev))
> +			q = qdisc_create(dev, dev_ingress_queue(dev), p,
> +					 tcm->tcm_parent, tcm->tcm_parent,
> +					 tca, &err);
> +		else
> +			err = -ENOENT;
> +	} else {
>  		struct netdev_queue *dev_queue;
...

> Here is the V3 then.
> 
> [PATCH net-next V3] net: dynamic ingress_queue allocation
> 
> ingress being not used very much, and net_device->ingress_queue being
> quite a big object (128 or 256 bytes), use a dynamic allocation if
> needed (tc qdisc add dev eth0 ingress ...)
> 
> dev_ingress_queue(dev) helper should be used only with RTNL taken.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> V3: add rcu notations & address Jarek comments
>  include/linux/netdevice.h |    2 -
>  include/linux/rtnetlink.h |    8 ++++++
>  net/core/dev.c            |   34 ++++++++++++++++++++++-------
>  net/sched/sch_api.c       |   42 ++++++++++++++++++++++++------------
>  net/sched/sch_generic.c   |   12 ++++++----
>  5 files changed, 71 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ceed347..92d81ed 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -986,7 +986,7 @@ struct net_device {
>  	rx_handler_func_t	*rx_handler;
>  	void			*rx_handler_data;
>  
> -	struct netdev_queue	ingress_queue; /* use two cache lines */
> +	struct netdev_queue __rcu *ingress_queue;
>  
>  /*
>   * Cache lines mostly used on transmit path
> diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
> index 68c436b..0bb7b48 100644
> --- a/include/linux/rtnetlink.h
> +++ b/include/linux/rtnetlink.h
> @@ -6,6 +6,7 @@
>  #include <linux/if_link.h>
>  #include <linux/if_addr.h>
>  #include <linux/neighbour.h>
> +#include <linux/netdevice.h>
>  
>  /* rtnetlink families. Values up to 127 are reserved for real address
>   * families, values above 128 may be used arbitrarily.
> @@ -769,6 +770,13 @@ extern int lockdep_rtnl_is_held(void);
>  #define rtnl_dereference(p)					\
>  	rcu_dereference_check(p, lockdep_rtnl_is_held())
>  
> +static inline struct netdev_queue *dev_ingress_queue(struct net_device *dev)
> +{
> +	return rtnl_dereference(dev->ingress_queue);

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

I think you should also add a comment here why this rcu is used, and
that it changes only once in dev's liftime.

Jarek P.

PS: checkpatched or not checkpatched, that is the question... ;-)