| Message ID | 1440202856-1645-4-git-send-email-phil@nwl.cc |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 22 Aug 2015 02:20:56 +0200 Phil Sutter <phil@nwl.cc> wrote: > When removing the root qdisc, the interface should fall back to noqueue > as the 'real' minimal qdisc instead of the default one. I worry this behavior could break existing scripts. I prefer the idea of allowing tc command to assign noqueue (to any device). This makes the action explicit for the user, instead of being a side-effect of removing a qdisc. (and does not break backward compat)
On Sun, 23 Aug 2015 20:44:42 +0200 Jesper Dangaard Brouer <brouer@redhat.com> wrote: > On Sat, 22 Aug 2015 02:20:56 +0200 > Phil Sutter <phil@nwl.cc> wrote: > > > When removing the root qdisc, the interface should fall back to noqueue > > as the 'real' minimal qdisc instead of the default one. > > I worry this behavior could break existing scripts. You would break OpenWRT package "qos-scripts", specifically: https://github.com/openwrt-mirror/openwrt/blob/master/package/network/config/qos-scripts/files/usr/bin/qos-stop Which cleans-up/clear the qdisc setup by removing the root qdisc, assuming and depending on the default qdisc is re-assigned. > I prefer the idea of allowing tc command to assign noqueue (to any > device). This makes the action explicit for the user, instead of being > a side-effect of removing a qdisc. (and does not break backward compat)
Hi Jesper, On Sun, Aug 23, 2015 at 08:53:57PM +0200, Jesper Dangaard Brouer wrote: > On Sun, 23 Aug 2015 20:44:42 +0200 > Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > > On Sat, 22 Aug 2015 02:20:56 +0200 > > Phil Sutter <phil@nwl.cc> wrote: > > > > > When removing the root qdisc, the interface should fall back to noqueue > > > as the 'real' minimal qdisc instead of the default one. > > > > I worry this behavior could break existing scripts. > > You would break OpenWRT package "qos-scripts", specifically: > https://github.com/openwrt-mirror/openwrt/blob/master/package/network/config/qos-scripts/files/usr/bin/qos-stop Thanks for pointing this out! > Which cleans-up/clear the qdisc setup by removing the root qdisc, > assuming and depending on the default qdisc is re-assigned. OK. Since the premise of the whole thing is to not break existing scripts, this sadly tears down my approach. > > I prefer the idea of allowing tc command to assign noqueue (to any > > device). This makes the action explicit for the user, instead of being > > a side-effect of removing a qdisc. (and does not break backward compat) I will give this another go. What I didn't like was that after attaching noqueue, tc would output nothing when asked to show the attached qdisc - which is of debatable correctness at least. But maybe that's just a user space problem I could address separately. Cheers, Phil -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 224374c..3b2cf30 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -839,7 +839,7 @@ skip: dev->qdisc, new); if (new && !new->ops->attach) atomic_inc(&new->refcnt); - dev->qdisc = new ? : &noop_qdisc; + dev->qdisc = new ? : &noqueue_qdisc; if (new && new->ops->attach) new->ops->attach(new); diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 68df721..ecc369b 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -718,9 +718,9 @@ struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue, /* ... and graft new one */ if (qdisc == NULL) - qdisc = &noop_qdisc; + qdisc = &noqueue_qdisc; dev_queue->qdisc_sleeping = qdisc; - rcu_assign_pointer(dev_queue->qdisc, &noop_qdisc); + rcu_assign_pointer(dev_queue->qdisc, qdisc); spin_unlock_bh(root_lock);
When removing the root qdisc, the interface should fall back to noqueue as the 'real' minimal qdisc instead of the default one. Therefore dev_graft_qdisc() has to be adjusted to assign noqueue if NULL was passed as new qdisc, and qdisc_graft() needs to assign noqueue to dev->qdisc instead of noop to prevent dev_activate() from attaching default qdiscs to the interface. Note that it is also necessary to have dev_graft_qdisc() set dev_queue->qdisc to the new qdisc instead of (unconditionally) noop. I don't know why this was there at all (originates from pre-git time), but it seems wrong to me. It could be worked around by droping the extra check for noqueue in transition_one_qdisc(), maybe with unintended side-effects. Signed-off-by: Phil Sutter <phil@nwl.cc> --- net/sched/sch_api.c | 2 +- net/sched/sch_generic.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)