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

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]

Message-ID: <20150729220812.GD15554@orbit.nwl.cc>
Date:	Thu, 30 Jul 2015 00:08:12 +0200
From:	Phil Sutter <phil@....cc>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org, Jesper Dangaard Brouer <brouer@...hat.com>,
	Cong Wang <cwang@...pensource.com>,
	David Miller <davem@...emloft.net>
Subject: Re: [net-next PATCH 2/3] net: sched: set tx_queue_len to default
 when changing noqueue device's qdisc

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