[PATCH -next] cbq: remove only caller of qdisc->drop()

[PATCH -next] cbq: remove only caller of qdisc->drop()

[Florian Westphal]

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);
 }
-- 
2.7.3

Re: [PATCH -next] cbq: remove only caller of qdisc->drop()

[Eric Dumazet]

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.

Re: [PATCH -next] cbq: remove only caller of qdisc->drop()

[Florian Westphal]

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

Re: [PATCH -next] cbq: remove only caller of qdisc->drop()

[David Miller]

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.