Re: [PATCH v2 net-next 2/2] tcp: add CDG congestion control

[Yuchung Cheng <ycheng@google.com>]

On Tue, Jun 9, 2015 at 3:41 PM, Kenneth Klette Jonassen
<kennetkl@ifi.uio.no> wrote:
> On Tue, Jun 9, 2015 at 7:44 AM, Yuchung Cheng <ycheng@google.com> wrote:
>> On Mon, Jun 8, 2015 at 10:43 AM, Kenneth Klette Jonassen
>> <kennetkl@ifi.uio.no> wrote:
> …
>
>>> +enum cdg_state {
>>> +       CDG_UNKNOWN = 0,
>>> +       CDG_FULL    = 0,
>> why duplicate states?
>
> We explicitly infer a full or non-full queue in tcp_cdg_grad().
>
> The unknown state is semantically different; it is before we infer
> anything about the network.
>
> We could change the full state to have its own value. (It does not
> matter in the current code, but it could be useful for TCP_CC_INFO.)
>
>>> +       if (hystart_detect & 1) {
>> Define 1 and 2 like cubic does?
>>> +       prior_snd_cwnd = tp->snd_cwnd;
>>> +       tcp_reno_cong_avoid(sk, ack, acked);
>>> +
>>> +       if (ca->shadow_wnd) {
>>> +               u32 incr = tp->snd_cwnd - prior_snd_cwnd;
>>> +
>>> +               ca->shadow_wnd = max(ca->shadow_wnd, ca->shadow_wnd + incr);
>>  u32 shadow_wnd may overflow?! there might be some bugs ...
>
> CDG can potentially operate without losses over some time, so
> shadow_wnd can reach U32_MAX on normal links.
> The max() should revert any increment that causes overflow/wrapping of
> shadow_wnd.
the situation you described could only happen if we didn't have the
application-limited check  (i.e., tcp_is_cwnd_limited() in tcp_reno_cong_avoid).
I am really doubtful, even tho having a max won't hurt anything.

>
> We limit shadow_wnd to min(shadow_wnd / 2, snd_cwnd) in ssthresh().
>
>>> +       if (ca->state == CDG_BACKOFF)
>>> +               return max(2U, (tp->snd_cwnd * min(1024U, backoff_beta)) >> 10);
>>> +
>>> +       if (ca->state == CDG_NONFULL && use_tolerance) {
>>> +               bool is_ecn = (tp->prior_ssthresh == 0);
>>> +
>>> +               if (!is_ecn) {
>>> +                       tp->ecn_flags &= ~TCP_ECN_DEMAND_CWR;
>>> +                       return tp->snd_cwnd;
>>> +               }
>> I might be missing something here: cdg_backoff also calls
>> tcp_enter_cwr() that clears prior_ssthresh but it's not triggered by
>> ECE marks?
>
> That is ok. We set ca->state = CDG_BACKOFF in tcp_cdg_backoff() to
> distinguish a delay backoff.
>
>>
>> if the intention is to use loss tolerance only when undo is enabled
>> why not just check undo_marker?
>
> The intention is to not use loss tolerance if we receive an ECN signal.
>
> Inferring ECN from (prior_ssthresh == 0) is far from perfect of
> course, but it is always cleared when receiving ECN.
>
> In tcp_enter_loss(), undo_marker is set after calling ssthresh(). We
> could change that, but undo_marker is also imperfect for detecting
> ECN.
>
> If in doubt, let us remove the ECN check now. Then in a later patch
> set, we could try passing flag to ssthresh like you suggested. We
> should also look at tcp_dctcp if we do that.
+1 to split ecn handling in a separate (future) patch

>
>>> +       if (use_shadow)
>>> +               ca->shadow_wnd = min(ca->shadow_wnd >> 1, tp->snd_cwnd);
>>> +       else
>>> +               ca->shadow_wnd = 0;
>> it'd be good to not reset shadow_wnd, but only use it if use_shadow.
>> so we can monitor shadow_wnd
>> in TCP_CC_INFO in the future.
>
> Ok, then we should always set/maintain a shadow_wnd.