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

> I am sending this as RFC since I have not yet deeply tested this. It makes
> the exchange of NN options in established state conform to RFC 4340, 6.6.1
> and thus actually is a bug fix.

I now had a chance to put this patch through extensive tests. I had to
modify two things about the patch to get good performance:
1)CCID2 now checks the congestion window against the currently
negotiating ack ratio value to determine whether to change the ack ratio
after an RTO, an idle period, or a loss. This prevents a problem where
an RTO or loss or idle period occurs and the ack ratio is less than the
congestion window but is in the process of being negotiated larger. That
negotiation will reach the sender on the first packet sent and RTOs will
result.
The primary change is adding a function called
dccp_feat_get_nn_next_val() to feat.c. This function returns the value
of the NN feature indicated that is is the process of being negotiated.
If the feature is not being negotiated, then the current value is
returned.
2)In a situation where the ack ratio has to be reduced because of an
RTO, idle period, or loss, CCID2 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)

Below is an improved patch. What are your thoughts on these changes?

Samuel Jero
Internetworking Research Group
Ohio University


>>>>>>>>>>>>>>>>>>>>>>>>>Patch<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
dccp: Only activate NN values after receiving the Confirm option

This defers changing local values using exchange of NN options in
established connection state by only activating the value after
receiving the Confirm option, as mandated by RFC 4340, 6.6.1.

Signed-off-by: Samuel Jero

| I now had a chance to put this patch through extensive tests. I had to
| modify two things about the patch to get good performance:
| 1) [...] The primary change is adding a function called
|     dccp_feat_get_nn_next_val() to feat.c.
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.

| 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.

| Below is an improved patch. What are your thoughts on these changes?
| 
I have uploaded your version of the patch to the test tree, 

http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=commitdiff;h=126ba220483bb990cf2c4b625464c89909583d3c

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?
 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);
 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.
    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).

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.

Once you are ok with the edits, I will add your Signed-off-by.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> 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