Re: dccp test-tree [RFC] [Patch 1/1] dccp: Only activate NN values after receiving the Confirm option

[Samuel Jero <sj323707@ohio.edu>]

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

> Well done, this looks good. I did some minor editing:
>  * whitespace/formatting/comments,
>  * simplification/subsumption,
>  * function should not be called for non-NN or non-known
>    feature, hence turned that into a DCCP_BUG() condition.

Okay

> 
> | 2)In a situation where the ack ratio has to be reduced because of an
> |    RTO, idle period, or loss, CCID-2 now sets the ack ratio to half of the
> |    congestion window (or 1 if that's zero) instead of to the congestion
> |    window. This should reduce the problems if one ack is lost (we have to
> |    lose two acks to not acknowledge an entire congestion window and trigger
> |    RTO)
> | 
> I think this makes for a separate patch, and it would be good to commentify
> the above into the code; please also see 3(b) below.

Separate patch coming shortly. Will add comment describing the
situation.

> Some work still remains to be done:
> 
>  1) Since ccid2_ack_ratio_next(sk) is just a wrapper around
>     dccp_feat_get_nn_next_val(sk, DCCPF_ACK_RATIO), ok to
>     use this instead?

It's just fine to use dccp_feat_get_nn_next_val() instead. My primary
reason for creating ccid2_ack_ratio_next() was to keep line lengths
down.

>  2) Analogously, for the local sequence window feature the
>     dccp_feat_get_nn_next_val() is not used, it uses the
>     current value:
>     if (val != dp->dccps_l_seq_win)
> 	dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW, val);

That should also be updated to use dccp_feat_get_nn_next_val(sk,
DCCPF_SEQUENCE_WINDOW)

>  3) There is room for some refactoring:
>     a) dccp_feat_signal_nn_change() always implies also in part
>        dccp_feat_get_nn_next_val(): if the latter function returns
>        the same value as the supposedly 'new' one, it is not
>        necessary to start a new negotiation. So all the repeated
>        tests could be folded into that function.


The problem here is that the ack ratio should only be changed after a
loss, idle period, etc if the new cwnd is going to be less than the
(negotiating) ack ratio. We need to call dccp_feat_get_nn_next_val() to
decide whether we need to adjust the ack ratio or not.

We don't want to change the ack ratio every time we have a loss, etc.
Doing so will result in pointless negotiations and more fluctuations in
the ack ratio, neither of which is desirable.

>     b) The following pattern appears three times in ccid2.c:
> 	if (ccid2_ack_ratio_next(sk) > hc->tx_cwnd)
> 		ccid2_change_l_ack_ratio(sk, hc->tx_cwnd/2 ? : 1U);
>        Perhaps this can, as some other parts of this patch set, be
>        refactored (e.g. the CCID-2 part is already a separate patch).

I'll create a function for this code. Coming in separate patch.
> 
> Other than the minor edits I have left your patch as is, i.e. I have
> not yet performed changes (1) and (2), awaiting your opinion on that.

Go ahead with 1) and 2).  I'll send out a new patch for 3 (b) shortly.

Samuel Jero
Internetworking Research Group
Ohio University

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]