Subject: comments on draft-balakrishnan-cm-02.txt

From: Vern Paxson <[email-masked]>
Date: Fri, 16 Jun 2000 15:47:33 PDT

>    FLOW 
> 	A stream of packets that all share the same source and
>         destination IP address, IP type-of-service, transport
>         protocol, and source and destination transport port numbers.

This term is too easily confused with "macroflow" (there are places in the
document where this happens).  The document should just use "stream", which
in its definition is noted as being in one-to-one correspondence with the
notion of "flow", anyway.

>    MACROFLOW
>         A group of flows that uses the same congestion management and
>         scheduling algorithms, and shares congestion state
>         information. Flows destined to different receivers MUST belong
>         to different macroflows.

I don't think the definition of macroflow should hardwire this last
requirement.  We may eventually wind up with mechanisms that unify flows
across multiple destinations.

This could be fixed by simply adding the word "Currently" at the
beginning of the sentence, to flag that this notion may change.

>    CONGESTION WINDOW (cwnd)
>         A CM state variable that modulates the amount of outstanding
>         data between sender and receiver.

Should there also be a definition for ssthresh?

>    transmit a heterogeneous types of data to receivers, ranging from

Delete 'a'.

>    implementing the algorithms in [Jacobson88, Stevens97]), the

Citation to Stevens97 should be updated to RFC 2581.  (This occurs
elsewhere, too.)

>    A future document will
>    describe the sender-receiver protocol and header formats that will
>    handle applications that do not incorporate their own feedback to
>    the CM.  (This document focuses on well-behaved applications.)

Could delete the parenthetical comment, or else modify it to make
it clear that "This document" means the current one, not the future
one that was just discussed.

> 4.      CM API

It's important that all of the discussion of APIs in this document
emphasizes that we're referring to *abstract* APIs.

>    1. Open: All applications MUST call cm_open(u32 dst) before using
>       the CM API.  dst is the 32-bit IPv4 address.  This returns an

There needs to be an IPv6 interface, too.

>    2. Close: When a flow terminates, the application SHOULD invoke
>       cm_close(i32 cm_flowid) to inform the CM about the termination
>       of the flow.

Per the comments above, change cm_flowid to cm_streamid.

>       information may either be statically configured, or obtained via
>       path MTU discovery [Mogul90].

Shouldn't doing PMTU-D be a SHOULD?

>    If a stream
>    does not transmit any data upon a cmapp_send() callback invocation,
>    it SHOULD call cm_notify(dst, 0) to allow the CM to permit other
>    flows in the macroflow to transmit data.

Why give it any sort of leeway here?  If the the app doesn't call
cm_notify(), what ensures that the bookkeeping will stay consistent?
(Or is the SHOULD to emphasize that correct operation of the controller
shouldn't rely on the didn't-send notification, because it's an easy
mistake for the app to make?)

>    One of the decisions the CM needs to make is the granularity at a

"at a" should be "at which a"

>    includes a Congestion Controller and a Scheduler, with well-defined
>    interfaces exported by them.

(here's another place that *abstract* needs to be mentioned)

>    The control algorithm decides when and how
>    much data can be transmitted by a flow.

(here's an instance where "flow" should be "macroflow")

>    It uses application
>    notifications (Section 4.3) from concurrent streams on the same
>    macroflow to build up information about the congestion state of the
>    different network paths.

It's not clear what's meant by "different network paths" here.  Different
paths taken by the elements of the same macroflow?  Or is this simply a
comment that there will be multiple macroflows?  If the latter, then I suggest
deleting it.

>    The congestion control module MUST implement the following
>    interfaces (these are not directly visible to applications; they
>    are within the context of a macroflow):

If they're not directly visible, then why *must* the congestion control
module implement them?  If it's to provide for modular interoperability
with different, separately-developed schedulers, then that should be stated.

Also, the parenthetical comment is important and should be more emphasized.
Otherwise the reader gets confused to how these routines are in fact different
from the ones discussed earlier.

>    - void query(u64 *rate, u32 *srtt): This function returns the
>      estimated rate (in bits per second) and smoothed round trip time
>      (in microseconds) for the macroflow.

Shouldn't it also return rttdev?

>    - void schedule(u32 num_bytes): When the congestion control module
>      determines that data can be sent, the schedule() routine MUST be
>      called with the number of bytes that can be sent.

This MUST reads a little funny to me.  Is the point that it shouldn't be
calle with *no more* bytes than the number that can be sent?  If so,
then phrasing it "with no more than the number ..." would make it read
a little better.

>      congestion control provides the information to satisfy an

"control" should be "controller"

>    - void notify(u32 nsent): This interface is used to notify the
>      scheduler module whenever data is sent by a CM application. The
>      nsent parameter indicates the number of bytes just sent by the
>      application.

Shouldn't the call include the flowid (streamid)?  That would simplify the
scheduler's state management.

>    outstanding window (tcp_ownd) estimate. Whenever, data is sent from

Delete comma.

>      perform all non-congestion checks (nagle, receiver advertised

Capitalize Nagle.

>      4. The cmapp_send() callback for TCP is set to a simple output
>      routine.

Is this really that simple?  Doesn't it need to compute TCP header fields,
possibly timestamps, etc?

>    The IP output routines on the host calls cm_notify() when the data
>    is actually sent out.

How does it know which flowid/streamid is responsible?

>      that a packet was lost and that at least 3 packets reached to

I think "arrived" is more clear than "reached".

>      Therefore, it calls
>      cm_update(flowid, 4 * avg_pkt_size, 3 * avg_pkt_size,
>      CM_TRANSIENT, rtt).

This needs careful explanation.  Also, why is it okay to use avg_pkt_size
instead of computing the actual sizes?  If this is a general principle,
then there needs to be a section explaining it as such.

>      5. Upon the arrival of a partial acknowledgment (one that does
>      not exceed that last segment transmitted at the time the loss

"that last segment" should be "the last segment".  Or should this 
be "the highest segment"?

>    When the source first starts, it uses the cm_query() call to get an
>    initial estimate of network bandwidth and delay.

But there won't actually be an initial estimate available, will there?
(unless some other streams have already been active)

>    To use the CM, the application must incorporate feedback from the
>    receiver.

Seems this "must" should be a MUST.

> 7.      Security considerations
> 
>    The provides many of the same services that the congestion control
        ^CM

>    congestion, an attacker reduce the performance observed by

add "can" before "reduce"