Re: [net-next PATCH 2/3] net: sched: set tx_queue_len to default when changing noqueue device's qdisc

[Phil Sutter <phil@nwl.cc>]

On Wed, Jul 29, 2015 at 11:10:45PM +0200, Eric Dumazet wrote:
> On Wed, 2015-07-29 at 22:51 +0200, Phil Sutter wrote:
[...]
> > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > index f06aa01..79b8900 100644
> > --- a/net/sched/sch_api.c
> > +++ b/net/sched/sch_api.c
> > @@ -1315,6 +1315,10 @@ graft:
> >  			qdisc_destroy(q);
> >  		return err;
> >  	}
> > +	/* circumvent noqueue hack for virtual interfaces if
> > +	 * user desires to use a qdisc on it */
> > +	if (!dev->tx_queue_len)
> > +		dev->tx_queue_len = CONFIG_DEFAULT_TX_QUEUE_LEN;
> >  
> >  	return 0;
> >  }
> 
> This is still broken for htb : htb_init() would run before this,
> and htb_init() does :
> 
> 
> q->direct_qlen = qdisc_dev(sch)->tx_queue_len;

Oh, indeed. But the two lines following it read:

| if (q->direct_qlen < 2) /* some devices have zero tx_queue_len */
| 	q->direct_qlen = 2;

So there is a workaround already in place. Scrolling through the other
implementations it looks like they either ignore tx_queue_len altogether
or have fallbacks for when it's zero already in place. Looks like I was
fixing a pfifo-specific problem in a generic way.

> Really, I am not sure this can be changed. qdisc scripts are mostly
> written by skilled people knowing if txqueuelen needs to be changed or
> not...
> 
> Change the kernel behavior and scripts will break.

I understand that, although I personally wouldn't rely upon such
inconsistent defaults if I were aware of them. Anyway, this series tries
to leave the skilled people alone whilst focussing on the potentially
less skilled ones using veth with default settings. In that regard
Florian's (and now also David's) suggestions to have a separate flag for
drivers defaulting to noqueue and leaving tx_queue_len alone might be a
much better choice.

I'll try that path instead and see where it leads me. Thanks a lot for
the quick help!

Cheers, Phil