Subject: [tsvwg] A review of draft-ietf-tsvwg-l4s-arch-09

From: Vidhi Goel <[email-masked]>
Date: Mon, 21 Jun 2021 21:02:39 -0700

Dear L4S authors,

I have a few comments on the architecture draft.

1. Section 4, Protocols
 >> much more frequent signals--too often to require an equivalent
    excessive degree of drop from non-ECN flows;

I am a little confused by this statement. Is it saying that the frequent signaling in L4S is too often that it is equivalent to the “drop” signal in case of non-ECN flows? Or does the too often ECN signal requires an a certain degree of drop from non-ECN? 
I think it means the former but still bit a confusing to read.

Similar question about the second bullet, too soon for L4S here means it will result in the same effect as drop for non-ECN, right?
 >> immediately tracking every fluctuation of the queue--too soon
    to warrant dropping packets from non-ECN flows.

2. Section 4, Network
  >> Then the Classic
     AQM such as CoDel or PIE is applied to non-ECN or ECT(0) packets,
     while the shallow threshold is applied to ECT(1) packets, to give
     sub-millisecond average queue delay.
If the bottleneck with CoDel has both classic and L4S packets in the queue at the same time, then a “shallow threshold” for L4S can’t guarantee sub-ms queue delay, right?

3. Section 4, Hosts
    >> Also the L4S ECN part of
    BBRv2 [I-D.cardwell-iccrg-bbr-congestion-control <https://datatracker.ietf.org/doc/html/draft-ietf-tsvwg-l4s-arch-09#ref-I-D.cardwell-iccrg-bbr-congestion-control>] is a scalable
    congestion control intended for the TCP and QUIC transports,
    amongst others.

The BBR draft has no mention of L4S ECN so far. Are we expecting an updated BBR draft and reference? There is another reference in Section 5.2. 

4. Section 5.1
    >> The consequent smaller amplitude sawteeth
    fit between a very shallow marking threshold and an empty
    queue, so queue delay variation can be very low, without risk
    of under-utilization.

Is there a description of what "very shallow marking threshold” means in terms of time or anything else? I think it would be good to quantify this term, even if it is approximate.

5. Section 5 has a lot of redundant info, perhaps it can be shortened. Some of it can be combined with Section 3 - terminology.

6. Section 5.2
   >> The L4S architecture does not require the IETF to commit to
    one approach over the other, because it supports both, so that
    the market can decide.

Minor suggestion: s/market/industry

I would really appreciate if you could re-organize Section 5 as some of the content in that section was previously covered. If needed, I can work with you on this.

Thanks,
Vidhi

---

From: Neal Cardwell <[email-masked]>
Date: Tue, 22 Jun 2021 16:33:45 -0400

On Tue, Jun 22, 2021 at 12:03 AM Vidhi Goel <vidhi_goel=
[email-masked]> wrote:

> The BBR draft has no mention of L4S ECN so far. Are we expecting an
> updated BBR draft and reference? There is another reference in Section 5.2.
>
>
If it's desired to have references for BBRv2 that cover the topic of ECN,
in order to avoid having dependencies on future BBR draft updates, it may
make sense to reference already-existing documentation about BBRv2. Some
possible options:

(1) BBRv2 algorithm high-level overview:

https://datatracker.ietf.org/meeting/104/materials/slides-104-iccrg-an-update-on-bbr-00

(2) the BBRv2 alpha release for Linux TCP:
  https://github.com/google/bbr/blob/v2alpha/README.md

(3) code from the BBRv2 alpha release for Linux TCP:
  https://github.com/google/bbr/blob/v2alpha/net/ipv4/tcp_bbr2.c

Thanks,
neal

---

From: Bob Briscoe <[email-masked]>
Date: Tue, 22 Jun 2021 22:15:47 +0100

Vidhi,

Thank you for this constructive review.
Inline tagged [BB]...

On 22/06/2021 05:02, Vidhi Goel wrote:
> I am a little confused by this statement. Is it saying that the 
> frequent signaling in L4S is too often that it is equivalent to the 
> “drop” signal in case of non-ECN flows? Or does the too often ECN 
> signal requires an a certain degree of drop from non-ECN?
> Similar question about the second bullet, too soon for L4S here means 
> it will result in the same effect as drop for non-ECN, right?

[BB] Yes, I admit I was trying to pack too much into the bullets. Does 
the following make better sense?:

    a.                                        ... responded to by hosts.
        L4S needs networks and hosts to support a more fine-grained
        meaning for each ECN signal that is less severe than a drop, so
        that the L4S signals:

        *  can be much more frequent;

        *  can immediately track every fluctuation of the queue.

        A drop is both an impairment and a congestion signal, so while
        ECN was equivalent to drop, it could not be used too frequently
        or too immediately without non-ECN flows having to be subjected
        to an equivalent degree of loss, and therefore excessive
        impairment.  To enable L4S,...

> If the bottleneck with CoDel has both classic and L4S packets in the 
> queue at the same time, then a “shallow threshold” for L4S can’t 
> guarantee sub-ms queue delay, right?

[BB] Yes, we didn't make the modification clear. Is this clearer:

    b.  A scheduler with per-flow queues such as FQ-CoDel or FQ-PIE can
        be used for L4S by simply modifying the classifiers in the
        existing design.  For instance within each queue of an FQ-CoDel
        system, as well as a CoDel AQM, there is typically also ECN
        marking at an immediate (unsmoothed) shallow threshold to support
        use in data centres (see Sec.5.2.7 of [RFC8290]).  This can be
        modified so that the shallow threshold is solely applied to
        ECT(1) packets.  Then if there is a flow of non-ECN or ECT(0)
        packets in the per-flow-queue, the Classic AQM such as CoDel or
        PIE is applied; while if there is a flow of ECT(1) packets in the
        queue, the shallower threshold is applied to give sub-millisecond
        average queue delay.  In addition, a mix of ECT(1) and ECT(0) or
        not-ECT packets (e.g. due to a hash collision or a VPN) can be
        classified into separate per-flow queues.

> 3. Section 4, Hosts
> >> Also the L4S ECN part of
> BBRv2 [I-D.cardwell-iccrg-bbr-congestion-control 
> <https://datatracker.ietf.org/doc/html/draft-ietf-tsvwg-l4s-arch-09#ref-I-D.cardwell-iccrg-bbr-congestion-control>] 
> is a scalable congestion control intended for the TCP and QUIC 
> transports, amongst others.

[BB] It is in the published implementation, but you're correct that the 
the draft is still at revision -00 from Jul 2017 and describes BBRv1 
without L4S. For now I have referred to the Linux implementation when 
talking specifically about v2, and I have referred to the draft when 
just discussing generally how the delay-based part of BBR works.

> Is there a description of what "very shallow marking threshold” means 
> in terms of time or anything else? I think it would be good to 
> quantify this term, even if it is approximate.

[BB] I've changed to "very shallow (~1 ms) marking threshold"
This quantification is discussed in much more detail in the AQM spec: 
draft-ietf-tsvwg-aqm-dual-queue-coupled

> 5. Section 5 has a lot of redundant info, perhaps it can be shortened. 
> Some of it can be combined with Section 3 - terminology.

[BB] I think some of the redundancy is between the Architecture 
Components Section (4) and the Rationale section (5) as well.
The rationale section is useful to avoid having to justify everything 
earlier on, so everything can be introduced more quickly before going 
into detail in a second pass. But sometimes you feel you have to justify 
something when you first introduce it. I'll try to be more ruthless.

I'm going to have to use old technology (print out the draft and write 
on it by hand), to try to see where there is duplication, and which 
duplicate to keep.

> Minor suggestion: s/market/industry

[BB] I know that is technically more accurate, because not everything is 
for sale, but "Let the market decide" is a common idiom used across the 
IETF whether or not something is for sale, e.g. RFC5218, RFC6670. If you 
feel strongly, I can change it, but for now I have just pt 'market' in 
quotes.

> I would really appreciate if you could re-organize Section 5 as some 
> of the content in that section was previously covered. If needed, I 
> can work with you on this.

[BB] Thanks. I'm starting work on that now. I'll be in touch if I need a 
second opinion on anything, and whatever, I'll run the diffs past you 
before posting.

And thank you for the review. Always useful to get new eyes on a draft 
to see things in plain sight that those of us familiar with a draft no 
longer notice.

Cheers

Bob

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

---

From: Vidhi Goel <[email-masked]>
Date: Tue, 22 Jun 2021 20:04:51 -0700

Hi Bob,

Thanks for taking care of my comments so promptly. I am satisfied with all your responses.
I agree that referring to the Linux implementation for BBRv2 would be more informative wherever the draft talks about implementation.

Looking forward to read the updated draft.

Thanks,
Vidhi

> On Jun 22, 2021, at 2:15 PM, Bob Briscoe <[email-masked]> wrote:
> 
> Vidhi, 
> 
> [BB] Yes, I admit I was trying to pack too much into the bullets. Does the following make better sense?:
> 
>> If the bottleneck with CoDel has both classic and L4S packets in the queue at the same time, 
>> then a “shallow threshold” for L4S can’t guarantee sub-ms queue delay, right?
> 
>> The BBR draft has no mention of L4S ECN so far. Are we expecting an updated BBR draft and reference? 
>> There is another reference in Section 5.2. 
> 
> [BB] It is in the published implementation, but you're correct that the the draft is still at revision -00 from Jul 2017 and describes BBRv1 without L4S. For now I have referred to the Linux implementation when talking specifically about v2, and I have referred to the draft when just discussing generally how the delay-based part of BBR works.
> 
>> 
>> 4. Section 5.1
>> Is there a description of what "very shallow marking threshold” means in terms of time or anything else? 
>> I think it would be good to quantify this term, even if it is approximate.
> 
> This quantification is discussed in much more detail in the AQM spec: draft-ietf-tsvwg-aqm-dual-queue-coupled
> 
>> 
> [BB] I think some of the redundancy is between the Architecture Components Section (4) and the Rationale section (5) as well.
> The rationale section is useful to avoid having to justify everything earlier on, so everything can be introduced more quickly before going into detail in a second pass. But sometimes you feel you have to justify something when you first introduce it. I'll try to be more ruthless.
> 
> I'm going to have to use old technology (print out the draft and write on it by hand), to try to see where there is duplication, and which duplicate to keep. 
> 
> 
>> 
>> 6. Section 5.2
> [BB] I know that is technically more accurate, because not everything is for sale, but "Let the market decide" is a common idiom used across the IETF whether or not something is for sale, e.g. RFC5218, RFC6670. If you feel strongly, I can change it, but for now I have just pt 'market' in quotes.
> 
>> 
> [BB] Thanks. I'm starting work on that now. I'll be in touch if I need a second opinion on anything, and whatever, I'll run the diffs past you before posting.
> 
> And thank you for the review. Always useful to get new eyes on a draft to see things in plain sight that those of us familiar with a draft no longer notice.
> 
> Cheers
> 
> 
> Bob
> 
> 
> 
>> 
>> Thanks,
>> Vidhi
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
> 
> -- 
> Bob Briscoe                               http://bobbriscoe.net/ <http://bobbriscoe.net/>

---

From: Neal Cardwell <[email-masked]>
Date: Tue, 22 Jun 2021 23:48:39 -0400

Thanks, Bob. All of your responses/updates sound good to me as well, for
what it's worth.

Best,
neal

On Tue, Jun 22, 2021 at 11:05 PM Vidhi Goel <vidhi_goel=
[email-masked]> wrote:

> I agree that referring to the Linux implementation for BBRv2 would be more
> informative wherever the draft talks about implementation.
>
> If the bottleneck with CoDel has both classic and L4S packets in the queue
> at the same time, then a “shallow threshold” for L4S can’t guarantee sub-ms
> queue delay, right?
>
>
> talking specifically about v2, and I have referred to the draft when just
> discussing generally how the delay-based part of BBR works.
>
> 4. Section 5.1
> [BB] I think some of the redundancy is between the Architecture Components
> Section (4) and the Rationale section (5) as well.
> earlier on, so everything can be introduced more quickly before going into
> detail in a second pass. But sometimes you feel you have to justify
> I'm going to have to use old technology (print out the draft and write on
> it by hand), to try to see where there is duplication, and which duplicate
> to keep.
>
>
> 6. Section 5.2
>
> And thank you for the review. Always useful to get new eyes on a draft to
> see things in plain sight that those of us familiar with a draft no longer
> notice.
>
> Cheers
>
>
> Bob
>
>
>
> Thanks,
>
> Vidhi
>
>
>
> --

---

From: Bob Briscoe <[email-masked]>
Date: Thu, 24 Jun 2021 18:50:21 +0100

Neal,

Thx - I'd already used the README as the ref for BBRv2, 'cos it contains 
a link to the CC module code.
  (and thx for the other mail confirming the other responses are AOK)

Bob

On 22/06/2021 21:33, Neal Cardwell wrote:
> On Tue, Jun 22, 2021 at 12:03 AM Vidhi Goel 
> <vidhi_goel=[email-masked] 
> <mailto:[email-masked]>> wrote:
>
>     3. Section 4, Hosts
>     >> Also the L4S ECN part of
>
>     The BBR draft has no mention of L4S ECN so far. Are we expecting
>     an updated BBR draft and reference? There is another reference in
>     Section 5.2.
>
>
> If it's desired to have references for BBRv2 that cover the topic of 
> ECN, in order to avoid having dependencies on future BBR draft 
> updates, it may make sense to reference already-existing documentation 
> about BBRv2. Some possible options:
>
> <https://datatracker.ietf.org/meeting/104/materials/slides-104-iccrg-an-update-on-bbr-00>
>
> <https://github.com/google/bbr/blob/v2alpha/README.md>
>
> <https://github.com/google/bbr/blob/v2alpha/net/ipv4/tcp_bbr2.c>
>
> Thanks,
> neal
>