Subject: [tsvwg] Minor comment on draft-ietf-tsvwg-aqm-dualq-coupled-22

From: Ingemar Johansson S <[email-masked]>
Date: Thu, 10 Mar 2022 14:55:21 +0000

Hi

I have one minor comment regarding the code in appendix A , page 45

“   *  Classic and coupled marking or dropping (i.e. based on p_C and
      p_CL from the PI controller) is not applied to a packet if the
      respective queue length in bytes is < 2 MTU (prior to enqueuing
      the packet or dequeuing it, depending on whether the AQM is
      configured to be applied at enqueue or dequeue);“

The word "respective" here led me to believe that, for drop to occur in the L-queue, the L-queue must have two packets or more. 
The correct assumption is however that the condition is based on the combined length of the L-queue and the C-queue. 
And this is indeed how it is implemented in the real code.
What is perhaps better is to replace "respective" with "aggregate" to avoid at least my confusion ?

Regards
Ingemar

==================================
Ingemar Johansson  M.Sc. 
Master Researcher

Ericsson Research
RESEARCHER
GFTL ER NAP NCM Netw Proto & E2E Perf
Labratoriegränd 11
977 53, Luleå, Sweden
Phone +46-1071 43042
SMS/MMS +46-73 078 3289
[email-masked]
www.ericsson.com

                    You can't start a fire
Worryin' about your little world fallin' apart
                       Bruce Springsteen
===================================

---

From: Sebastian Moeller <[email-masked]>
Date: Thu, 10 Mar 2022 16:35:30 +0100

Hi Ingemar,

> On Mar 10, 2022, at 15:55, Ingemar Johansson S <ingemar.s.johansson=[email-masked]> wrote:
> 
> Hi
> 

	As far as I can tell the code does calculate the overall reference mark/drop probability based on the maximum of the queueing delays of the l- and c-queue, if this probability exceeds an overload threshold drops are scheduled. See https://github.com/L4STeam/linux/blob/testing/net/sched/sch_dualpi2.c, calculate_probability(), and must_drop().

Which tests:
	if (sch->qstats.backlog < 2 * psched_mtu(qdisc_dev(sch)))
		return false;

But isn't sch just the struct Qdisc for the classic queue:

struct dualpi2_sched_data {
	struct Qdisc *l_queue;	/* The L4S LL queue */
	struct Qdisc *sch;	/* The classic queue (owner of this struct) */
[...]

So is the real code really taking the length of the L-queue into account here or not? My reading is it is not, what am I missing here?

Regards
	Sebastian

---

From: Sebastian Moeller <[email-masked]>
Date: Fri, 11 Mar 2022 12:48:26 +0100

Hi Oliver,

thanks for clearing that up. I guess I am simply not fluent enough in C, but given that the thing is called Dual queue, maybe the comment in the definition of dualpi2_sched_data could expand on that only sch is exposed and hence contains (partial) information for both queues? Again, this might just be my own reading problems and obvious to those skilled in kernel-code...

Regards
	Sebastian

> On Mar 11, 2022, at 12:00, Tilmans, Olivier (Nokia - BE/Antwerp) <[email-masked]> wrote:
> 
> Sebastian,
> 
>> 	As far as I can tell the code does calculate the overall reference
>> mark/drop probability based on the maximum of the queueing delays of the
>> l- and c-queue, if this probability exceeds an overload threshold drops
>> are scheduled. See
> 
> Indeed--the key aspect here is that this allows PI2 to raise its
> probability even in the absence of classic traffic, i.e., if there's too
> much unresponsive traffic in the L4S queue, so that it can switch to drop
> and get it under control.
> 
>> Which tests:
>> So is the real code really taking the length of the L-queue into account
>> here or not? My reading is it is not, what am I missing here?
> 
> The classic queue in itself is only the skb list inside the Qdisc struct.
> 
> That struct's qlen/backlog statistics however account for both classic
> and L4S, i.e., the backlog covers both queues. This is because only 'sch'
> is exposed to outside structures, so it needs to be consistent from the outside
> world, e.g., enqueue/dequeue must result in backlog increase/decrease so that
> consumers like HTB can do their job properly.
> See https://github.com/L4STeam/linux/blob/testing/net/sched/sch_dualpi2.c#L397 ;
> and/or other multi-queue qdiscs.
> 
> 
> Best,
> Olivier

---

From: "Tilmans, Olivier (Nokia - BE/Antwerp)" <[email-masked]>
Date: Fri, 11 Mar 2022 11:00:17 +0000

Sebastian,

Best,
Olivier

---

From: Bob Briscoe <[email-masked]>
Date: Sun, 13 Mar 2022 18:00:26 +0000

Ingemar,

Thank you. Yes, the Linux code suppresses marking if the backlog of the 
whole qdisc is below 2 MTU:
https://github.com/L4STeam/linux/blob/testing/net/sched/sch_dualpi2.c#L273
So, 'respective' is not the right word, and indeed 'aggregate' would be 
better. I've made the substitution in the editor's local copy.

Thx again.

Bob

-- 
________________________________________________________________
Bob Briscoe                               http://bobbriscoe.net/