| Message ID | 1465248027-21652-1-git-send-email-fw@strlen.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2016-06-06 at 23:20 +0200, Florian Westphal wrote: > since initial revision of cbq in 2004 iproute2 never implemented > support for TCA_CBQ_OVL_STRATEGY, which is what needs to be set to > activate the class->drop() call (TC_CBQ_OVL_DROP strategy must be > set by userspace). > > So lets remove this. We can even do this in a backwards compatible > way by switching ovl_drop to perform a dequeue+drop on the leaf. > > A followup commit can then remove all .drop qdisc methods since this > was the only caller. > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > On a related note, iproute2 doesn't support the TCA_CBQ_POLICE > attribute either. If we'd remove that too we could then get rid > of __parent and reshape_fail in struct Qdisc. > > However, in TCA_CBQ_POLICE case there is no way to replace > the functionality, i.e. we'd have to -EOPNOTSUPP or ignore > TCA_CBQ_POLICE if some other non-iproute2 tool presents it to us. > > AFAICS TCA_CBQ_POLICE doesn't work all that well even if userspace > would set it, one needs to: > > add a class > attach a qdisc to the class (default pfifo doesn't work as > q->handle is 0 and cbq_set_police() is a no-op in this case) > re-'add' the same class (tc class change ...) again > > user must also specifiy a defmap (e.g. 'split 1:0 defmap 3f'), since > this 'police' feature relies on its presence > the added qdisc (or the leaves) must be one of bfifo, pfifo, tbf or > netem as only these implement qdisc_reshape_fail() call. > > If all of these conditions are met, then instead of drop the leaf > qdiscs mentioned above would attempt to re-enqueue the skb in the > cbq TC_PRIO_BESTEFFORT class if their limit is reached. > > I think it would be safe to just ignore TCA_CBQ_POLICE and change the > qdiscs to drop right away. > > Does anyone have a reason to leave this in the tree? Thanks! > > diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c > index baafddf..df79791 100644 > --- a/net/sched/sch_cbq.c > +++ b/net/sched/sch_cbq.c > @@ -542,9 +542,14 @@ static void cbq_ovl_lowprio(struct cbq_class *cl) > > static void cbq_ovl_drop(struct cbq_class *cl) > { > - if (cl->q->ops->drop) > - if (cl->q->ops->drop(cl->q)) > - cl->qdisc->q.qlen--; > + struct sk_buff *skb = cl->q->ops->dequeue(cl->q); > + > + if (skb) { > + cl->deficit -= qdisc_pkt_len(skb); > + cl->qdisc->q.qlen--; > + qdisc_drop(skb, cl->qdisc); > + } > + > cl->xstats.overactions++; > cbq_ovl_classic(cl); > } A drop() is not equivalent to a dequeue() followed by qdisc_drop() for statistics. dequeue() will update stats of _sent_ packets/bytes, while drop() should not.
Eric Dumazet <eric.dumazet@gmail.com> wrote: > > static void cbq_ovl_drop(struct cbq_class *cl) > > { > > - if (cl->q->ops->drop) > > - if (cl->q->ops->drop(cl->q)) > > - cl->qdisc->q.qlen--; > > + struct sk_buff *skb = cl->q->ops->dequeue(cl->q); > > + > > + if (skb) { > > + cl->deficit -= qdisc_pkt_len(skb); > > + cl->qdisc->q.qlen--; > > + qdisc_drop(skb, cl->qdisc); > > + } > > + > > cl->xstats.overactions++; > > cbq_ovl_classic(cl); > > } > > A drop() is not equivalent to a dequeue() followed by qdisc_drop() for > statistics. > > dequeue() will update stats of _sent_ packets/bytes, while drop() should > not. Well, I could send patch to just remove cbq_ovl_drop completely, you can't configure this facility with iproute2. You are right of course, but is it really worth to have this? Not calling cl->q->ops->drop() in cbq would allow removal of ~300 LOC in qdiscs...
From: Florian Westphal <fw@strlen.de> Date: Mon, 6 Jun 2016 23:20:27 +0200 > since initial revision of cbq in 2004 iproute2 never implemented > support for TCA_CBQ_OVL_STRATEGY, which is what needs to be set to > activate the class->drop() call (TC_CBQ_OVL_DROP strategy must be > set by userspace). > > So lets remove this. We can even do this in a backwards compatible > way by switching ovl_drop to perform a dequeue+drop on the leaf. > > A followup commit can then remove all .drop qdisc methods since this > was the only caller. > > Signed-off-by: Florian Westphal <fw@strlen.de> I don't see anything, anywhere, using TCA_CBQ_OVL_STRATEGY nor the various strategy type values. iproute2 seems to notice the attribute in dumps, but doesn't do anything with it other than verify that it's size meets some minimum. It seems really safe to kill this thing off, flag an error if someone tries to set the attribute, and therefore kill off all of the non-default cbq_ovl_*() functions. > I think it would be safe to just ignore TCA_CBQ_POLICE and change the > qdiscs to drop right away. > > Does anyone have a reason to leave this in the tree? Thanks! The TCA_CBQ_POLICE situation looks similar to the TCA_CBQ_OVL_STRATEGY one to me. Thanks.
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c index baafddf..df79791 100644 --- a/net/sched/sch_cbq.c +++ b/net/sched/sch_cbq.c @@ -542,9 +542,14 @@ static void cbq_ovl_lowprio(struct cbq_class *cl) static void cbq_ovl_drop(struct cbq_class *cl) { - if (cl->q->ops->drop) - if (cl->q->ops->drop(cl->q)) - cl->qdisc->q.qlen--; + struct sk_buff *skb = cl->q->ops->dequeue(cl->q); + + if (skb) { + cl->deficit -= qdisc_pkt_len(skb); + cl->qdisc->q.qlen--; + qdisc_drop(skb, cl->qdisc); + } + cl->xstats.overactions++; cbq_ovl_classic(cl); }
since initial revision of cbq in 2004 iproute2 never implemented support for TCA_CBQ_OVL_STRATEGY, which is what needs to be set to activate the class->drop() call (TC_CBQ_OVL_DROP strategy must be set by userspace). So lets remove this. We can even do this in a backwards compatible way by switching ovl_drop to perform a dequeue+drop on the leaf. A followup commit can then remove all .drop qdisc methods since this was the only caller. Signed-off-by: Florian Westphal <fw@strlen.de> --- On a related note, iproute2 doesn't support the TCA_CBQ_POLICE attribute either. If we'd remove that too we could then get rid of __parent and reshape_fail in struct Qdisc. However, in TCA_CBQ_POLICE case there is no way to replace the functionality, i.e. we'd have to -EOPNOTSUPP or ignore TCA_CBQ_POLICE if some other non-iproute2 tool presents it to us. AFAICS TCA_CBQ_POLICE doesn't work all that well even if userspace would set it, one needs to: add a class attach a qdisc to the class (default pfifo doesn't work as q->handle is 0 and cbq_set_police() is a no-op in this case) re-'add' the same class (tc class change ...) again user must also specifiy a defmap (e.g. 'split 1:0 defmap 3f'), since this 'police' feature relies on its presence the added qdisc (or the leaves) must be one of bfifo, pfifo, tbf or netem as only these implement qdisc_reshape_fail() call. If all of these conditions are met, then instead of drop the leaf qdiscs mentioned above would attempt to re-enqueue the skb in the cbq TC_PRIO_BESTEFFORT class if their limit is reached. I think it would be safe to just ignore TCA_CBQ_POLICE and change the qdiscs to drop right away. Does anyone have a reason to leave this in the tree? Thanks!