[Cake] [PATCH] Keep an internal counter for total queue length

[Cake] [PATCH] Keep an internal counter for total queue length

[Toke Høiland-Jørgensen]

Since sch->q.qlen can be changed from outside cake, we can't rely on it for
break loops etc. So keep an internal counter that mirrors it and use that
for all checks.

This the issue with inifinte loops on multi-queue hardware.

Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 sch_cake.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/sch_cake.c b/sch_cake.c
index ec0b0f2..3667741 100644
--- a/sch_cake.c
+++ b/sch_cake.c
@@ -233,6 +233,10 @@ struct cake_sched_data {
 	u32		buffer_limit;
 	u32		buffer_config_limit;
 
+	/* we need an internal counter for total qlen since sch->q.qlen can be
+	 * modified by other parts of the qdisc infrastructure */
+	u32		tot_qlen;
+
 	/* indices for dequeue */
 	u16		cur_tin;
 	u16		cur_flow;
@@ -1523,6 +1527,7 @@ static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free)
 
 	__qdisc_drop(skb, to_free);
 	sch->q.qlen--;
+	q->tot_qlen--;
 
 	cake_heapify(q, 0);
 
@@ -1664,7 +1669,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		if (ktime_before(b->time_next_packet, now))
 			b->time_next_packet = now;
 
-		if (!sch->q.qlen) {
+		if (!q->tot_qlen) {
 			if (ktime_before(q->time_next_packet, now)) {
 				q->failsafe_next_packet = now;
 				q->time_next_packet = now;
@@ -1702,6 +1707,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			flow_queue_add(flow, segs);
 
 			sch->q.qlen++;
+			q->tot_qlen++;
 			slen += segs->len;
 			q->buffer_used += segs->truesize;
 			b->packets++;
@@ -1739,6 +1745,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			consume_skb(ack);
 		} else {
 			sch->q.qlen++;
+			q->tot_qlen++;
 			q->buffer_used      += skb->truesize;
 		}
 
@@ -1859,6 +1866,7 @@ static struct sk_buff *cake_dequeue_one(struct Qdisc *sch)
 		sch->qstats.backlog      -= len;
 		q->buffer_used		 -= skb->truesize;
 		sch->q.qlen--;
+		q->tot_qlen--;
 
 		if (q->overflow_timeout)
 			cake_heapify(q, b->overflow_idx[q->cur_flow]);
@@ -1893,7 +1901,7 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
 	u32 len;
 
 begin:
-	if (!sch->q.qlen)
+	if (!q->tot_qlen)
 		return NULL;
 
 	/* global hard shaper */
@@ -2092,12 +2100,12 @@ retry:
 	flow->deficit -= len;
 	b->tin_deficit -= len;
 
-	if (ktime_after(q->time_next_packet, now) && sch->q.qlen) {
+	if (ktime_after(q->time_next_packet, now) && q->tot_qlen) {
 		u64 next = min(ktime_to_ns(q->time_next_packet),
 			       ktime_to_ns(q->failsafe_next_packet));
 
 		qdisc_watchdog_schedule_ns(&q->watchdog, next);
-	} else if (!sch->q.qlen) {
+	} else if (!q->tot_qlen) {
 		int i;
 
 		for (i = 0; i < q->tin_cnt; i++) {
-- 
2.7.4

Re: [Cake] [PATCH] Keep an internal counter for total queue length

[Toke Høiland-Jørgensen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Since sch->q.qlen can be changed from outside cake, we can't rely on it for
> break loops etc. So keep an internal counter that mirrors it and use that
> for all checks.

Sent this as a patch to avoid stepping on your toes if you are working
on the code now :)

-Toke

Re: [Cake] [PATCH] Keep an internal counter for total queue length

[Jonathan Morton]

> On 6 Jul, 2018, at 3:00 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Toke Høiland-Jørgensen <toke@toke.dk> writes:
> 
>> Since sch->q.qlen can be changed from outside cake, we can't rely on it for
>> break loops etc. So keep an internal counter that mirrors it and use that
>> for all checks.
> 
> Sent this as a patch to avoid stepping on your toes if you are working
> on the code now :)

I'm handling it without using any extra permanent state, just making use of what's already there.  Take a look at latest commit.

 - Jonathan Morton

Re: [Cake] [PATCH] Keep an internal counter for total queue length

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

>> On 6 Jul, 2018, at 3:00 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>> 
>>> Since sch->q.qlen can be changed from outside cake, we can't rely on it for
>>> break loops etc. So keep an internal counter that mirrors it and use that
>>> for all checks.
>> 
>> Sent this as a patch to avoid stepping on your toes if you are working
>> on the code now :)
>
> I'm handling it without using any extra permanent state, just making
> use of what's already there. Take a look at latest commit.

Yup, that also fixes the infinite loop issue. I'm fine with doing it
this way; however, it doesn't handle the other places where we check
whether sch->q.qlen is nonzero. Are you sure that is not going to give
us more problems down the road? :)

-Toke

Re: [Cake] [PATCH] Keep an internal counter for total queue length

[Jonathan Morton]

> On 6 Jul, 2018, at 4:21 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
>> I'm handling it without using any extra permanent state, just making
>> use of what's already there. Take a look at latest commit.
> 
> Yup, that also fixes the infinite loop issue. I'm fine with doing it
> this way; however, it doesn't handle the other places where we check
> whether sch->q.qlen is nonzero. Are you sure that is not going to give
> us more problems down the road? :)

The other places are checking whether the queue is empty for the purposes of updating the shaper state.  They are therefore much less critical, and I think self-correcting.

 - Jonathan Morton

Re: [Cake] [PATCH] Keep an internal counter for total queue length

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

>> On 6 Jul, 2018, at 4:21 pm, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>> 
>>> I'm handling it without using any extra permanent state, just making
>>> use of what's already there. Take a look at latest commit.
>> 
>> Yup, that also fixes the infinite loop issue. I'm fine with doing it
>> this way; however, it doesn't handle the other places where we check
>> whether sch->q.qlen is nonzero. Are you sure that is not going to give
>> us more problems down the road? :)
>
> The other places are checking whether the queue is empty for the
> purposes of updating the shaper state. They are therefore much less
> critical, and I think self-correcting.

Right, cool. In that case I think we are good to go for another upstream
submission; I'll get that done later today :)

-Toke

Re: [Cake] [PATCH] Keep an internal counter for total queue length

[Dave Taht]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

I am curious as to the cpu and thruput difference between pfifo,  htb +
fqcodel v cake at these insane speeds.


On Fri, Jul 6, 2018, 3:27 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> Jonathan Morton <chromatix99@gmail.com> writes:
>
> >> On 6 Jul, 2018, at 4:21 pm, Toke Høiland-Jørgensen <toke@toke.dk>
> wrote:
> >>
> >>> I'm handling it without using any extra permanent state, just making
> >>> use of what's already there. Take a look at latest commit.
> >>
> >> Yup, that also fixes the infinite loop issue. I'm fine with doing it
> >> this way; however, it doesn't handle the other places where we check
> >> whether sch->q.qlen is nonzero. Are you sure that is not going to give
> >> us more problems down the road? :)
> >
> > The other places are checking whether the queue is empty for the
> > purposes of updating the shaper state. They are therefore much less
> > critical, and I think self-correcting.
>
> Right, cool. In that case I think we are good to go for another upstream
> submission; I'll get that done later today :)
>
> -Toke
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake
>

[-- Attachment #2: Type: text/html, Size: 1975 bytes --]

Re: [Cake] [PATCH] Keep an internal counter for total queue length

[Toke Høiland-Jørgensen]

Dave Taht <dave.taht@gmail.com> writes:

> I am curious as to the cpu and thruput difference between pfifo, htb +
> fqcodel v cake at these insane speeds.

Sure, actual test results at ludicrous speed is on my list :)

-Toke