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

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

[Gerrit Renker]

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.

>>>>>>>>>>>>>>>>>>>>>>>>> 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: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ccids/ccid2.c |    7 ++-----
 net/dccp/feat.c        |   13 ++++---------
 2 files changed, 6 insertions(+), 14 deletions(-)

--- a/net/dccp/feat.c
+++ b/net/dccp/feat.c
@@ -775,12 +775,7 @@ int dccp_feat_register_sp(struct sock *s
  * @sk: DCCP socket of an established connection
  * @feat: NN feature number from %dccp_feature_numbers
  * @nn_val: the new value to use
- * This function is used to communicate NN updates out-of-band. The difference
- * to feature negotiation during connection setup is that values are activated
- * immediately after validation, i.e. we don't wait for the Confirm: either the
- * value is accepted by the peer (and then the waiting is futile), or it is not
- * (Reset or empty Confirm). We don't accept empty Confirms - transmitted values
- * are validated, and the peer "MUST accept any valid value" (RFC 4340, 6.3.2).
+ * This function is used to communicate NN updates out-of-band.
  */
 int dccp_feat_signal_nn_change(struct sock *sk, u8 feat, u64 nn_val)
 {
@@ -805,9 +800,6 @@ int dccp_feat_signal_nn_change(struct so
 		dccp_feat_list_pop(entry);
 	}
 
-	if (dccp_feat_activate(sk, feat, 1, &fval))
-		return -EADV;
-
 	inet_csk_schedule_ack(sk);
 	return dccp_feat_push_change(fn, feat, 1, 0, &fval);
 }
@@ -1356,6 +1348,9 @@ static u8 dccp_feat_handle_nn_establishe
 		if (fval.nn != entry->val.nn)
 			return 0;
 
+		/* Only activate after receiving the Confirm option (6.6.1). */
+		dccp_feat_activate(sk, feat, local, &fval);
+
 		/* It has been confirmed - so remove the entry */
 		dccp_feat_list_pop(entry);
 
--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -105,7 +105,6 @@ static void ccid2_change_l_ack_ratio(str
 		return;
 
 	ccid2_pr_debug("changing local ack ratio to %u\n", val);
-	dp->dccps_l_ack_ratio = val;
 	dccp_feat_signal_nn_change(sk, DCCPF_ACK_RATIO, val);
 }
 
@@ -117,11 +116,9 @@ static void ccid2_change_l_seq_window(st
 		val = DCCPF_SEQ_WMIN;
 	if (val > DCCPF_SEQ_WMAX)
 		val = DCCPF_SEQ_WMAX;
-	if (val == dp->dccps_l_seq_win)
-		return;
 
-	dp->dccps_l_seq_win = val;
-	dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW, val);
+	if (val != dp->dccps_l_seq_win)
+		dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW, val);
 }
 
 static void ccid2_hc_tx_rto_expire(unsigned long data)

-- 

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

[Samuel Jero]

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

> 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
diff --git a/net/dccp/ccids/ccid2.c b/net/dccp/ccids/ccid2.c
--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -83,9 +82,13 @@ static int ccid2_hc_tx_send_packet(struct sock *sk, struct sk_buff *skb)
 	return CCID_PACKET_SEND_AT_ONCE;
 }
 
+static __u64 ccid2_ack_ratio_next(struct sock *sk)
+{
+	return dccp_feat_get_nn_next_val(sk, DCCPF_ACK_RATIO);
+}
+
 static void ccid2_change_l_ack_ratio(struct sock *sk, u32 val)
 {
-	struct dccp_sock *dp = dccp_sk(sk);
 	u32 max_ratio = DIV_ROUND_UP(ccid2_hc_tx_sk(sk)->tx_cwnd, 2);
 
 	/*
@@ -101,11 +104,10 @@ static void ccid2_change_l_ack_ratio(struct sock *sk, u32 val)
 	if (val > DCCPF_ACK_RATIO_MAX)
 		val = DCCPF_ACK_RATIO_MAX;
 
-	if (val == dp->dccps_l_ack_ratio)
+	if (val == ccid2_ack_ratio_next(sk))
 		return;
 
 	ccid2_pr_debug("changing local ack ratio to %u\n", val);
-	dp->dccps_l_ack_ratio = val;
 	dccp_feat_signal_nn_change(sk, DCCPF_ACK_RATIO, val);
 }
 
@@ -117,11 +119,9 @@ static void ccid2_change_l_seq_window(struct sock *sk, u64 val)
 		val = DCCPF_SEQ_WMIN;
 	if (val > DCCPF_SEQ_WMAX)
 		val = DCCPF_SEQ_WMAX;
-	if (val == dp->dccps_l_seq_win)
-		return;
 
-	dp->dccps_l_seq_win = val;
-	dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW, val);
+	if (val != dp->dccps_l_seq_win)
+		dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW, val);
 }
 
 static void ccid2_hc_tx_rto_expire(unsigned long data)
@@ -203,6 +203,10 @@ static void ccid2_cwnd_application_limited(struct sock *sk, const u32 now)
 	}
 	hc->tx_cwnd_used  = 0;
 	hc->tx_cwnd_stamp = now;
+
+	/* Avoid spurious timeouts resulting from Ack Ratio > cwnd */
+	if (ccid2_ack_ratio_next(sk) > hc->tx_cwnd)
+		ccid2_change_l_ack_ratio(sk, hc->tx_cwnd/2 ? : 1U);
 }
 
 /* This borrows the code of tcp_cwnd_restart() */
@@ -221,6 +225,10 @@ static void ccid2_cwnd_restart(struct sock *sk, const u32 now)
 
 	hc->tx_cwnd_stamp = now;
 	hc->tx_cwnd_used  = 0;
+
+	/* Avoid spurious timeouts resulting from Ack Ratio > cwnd */
+	if (ccid2_ack_ratio_next(sk) > hc->tx_cwnd)
+		ccid2_change_l_ack_ratio(sk, hc->tx_cwnd/2 ? : 1U);
 }
 
 static void ccid2_hc_tx_packet_sent(struct sock *sk, unsigned int len)
@@ -491,8 +531,8 @@ static void ccid2_congestion_event(struct sock *sk, struct ccid2_seq *seqp)
 	hc->tx_ssthresh  = max(hc->tx_cwnd, 2U);
 
 	/* Avoid spurious timeouts resulting from Ack Ratio > cwnd */
-	if (dccp_sk(sk)->dccps_l_ack_ratio > hc->tx_cwnd)
-		ccid2_change_l_ack_ratio(sk, hc->tx_cwnd);
+	if (ccid2_ack_ratio_next(sk) > hc->tx_cwnd)
+		ccid2_change_l_ack_ratio(sk, hc->tx_cwnd/2 ? : 1U);
 }
 
 static int ccid2_hc_tx_parse_options(struct sock *sk, u8 packet_type,
diff --git a/net/dccp/dccp.h b/net/dccp/dccp.h
index 26cba68..e8cf5a7 100644
--- a/net/dccp/dccp.h
+++ b/net/dccp/dccp.h
@@ -491,6 +491,7 @@ static inline int dccp_ack_pending(const struct sock *sk)
 }
 
 extern int  dccp_feat_signal_nn_change(struct sock *sk, u8 feat, u64 nn_val);
+extern __u64 dccp_feat_get_nn_next_val(struct sock *sk, u8 feat);
 extern int  dccp_feat_finalise_settings(struct dccp_sock *dp);
 extern int  dccp_feat_server_ccid_dependencies(struct dccp_request_sock *dreq);
 extern int  dccp_feat_insert_opts(struct dccp_sock*, struct dccp_request_sock*,
diff --git a/net/dccp/feat.c b/net/dccp/feat.c
index 9a8c2ba..bc26f33 100644
--- a/net/dccp/feat.c
+++ b/net/dccp/feat.c
@@ -775,12 +775,7 @@ int dccp_feat_register_sp(struct sock *sk, u8 feat, u8 is_local,
  * @sk: DCCP socket of an established connection
  * @feat: NN feature number from %dccp_feature_numbers
  * @nn_val: the new value to use
- * This function is used to communicate NN updates out-of-band. The difference
- * to feature negotiation during connection setup is that values are activated
- * immediately after validation, i.e. we don't wait for the Confirm: either the
- * value is accepted by the peer (and then the waiting is futile), or it is not
- * (Reset or empty Confirm). We don't accept empty Confirms - transmitted values
- * are validated, and the peer "MUST accept any valid value" (RFC 4340, 6.3.2).
+ * This function is used to communicate NN updates out-of-band.
  */
 int dccp_feat_signal_nn_change(struct sock *sk, u8 feat, u64 nn_val)
 {
@@ -805,9 +800,6 @@ int dccp_feat_signal_nn_change(struct sock *sk, u8 feat, u64 nn_val)
 		dccp_feat_list_pop(entry);
 	}
 
-	if (dccp_feat_activate(sk, feat, 1, &fval))
-		return -EADV;
-
 	inet_csk_schedule_ack(sk);
 	return dccp_feat_push_change(fn, feat, 1, 0, &fval);
 }
@@ -1356,6 +1348,9 @@ static u8 dccp_feat_handle_nn_established(struct sock *sk, u8 mandatory, u8 opt,
 		if (fval.nn != entry->val.nn)
 			return 0;
 
+		/* Only activate after receiving the Confirm option (6.6.1). */
+		dccp_feat_activate(sk, feat, local, &fval);
+
 		/* It has been confirmed - so remove the entry */
 		dccp_feat_list_pop(entry);
 
@@ -1374,6 +1369,39 @@ fast_path_failed:
 			 : DCCP_RESET_CODE_OPTION_ERROR;
 }
 
+/*
+* dccp_feat_get_nn_next_val  -  Get the currently negotiating value of an NN
+*				feature. If the feature isn't being currently
+*				negotiated, return it's current value.
+* @sk: DCCP socket of an established connection
+* @feat: NN feature number from %dccp_feature_numbers
+*/
+__u64 dccp_feat_get_nn_next_val(struct sock *sk, u8 feat)
+{
+	struct list_head *fn = &dccp_sk(sk)->dccps_featneg;
+	struct dccp_feat_entry *entry;
+	u8 type = dccp_feat_type(feat);
+
+	if (type == FEAT_UNKNOWN || type != FEAT_NN)
+		return 0;
+
+	entry = dccp_feat_list_lookup(fn, feat, 1);
+	if (entry != NULL)
+		return entry->val.nn;
+
+	switch (feat) {
+	case DCCPF_ACK_RATIO:
+		return dccp_sk(sk)->dccps_l_ack_ratio;
+	case DCCPF_SEQUENCE_WINDOW:
+		return dccp_sk(sk)->dccps_l_seq_win;
+	default:
+		dccp_pr_debug("Attempting to get value \
+						for unknown NN feature");
+	}
+return 0;
+}
+EXPORT_SYMBOL_GPL(dccp_feat_get_nn_next_val);
+
 /**
  * dccp_feat_parse_options  -  Process Feature-Negotiation Options
  * @sk: for general use and used by the client during connection setup

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

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

[Gerrit Renker]

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

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

[Samuel Jero]

[-- 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 --]

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

[Gerrit Renker]

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

Thank you for the comments, there is also some update (attached).


| > 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
| 
I revised this some more, the function remains the same, but it is shorter 
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=commitdiff;h=94da7d70003e8185fe189146f336db72c8fa1f0b

(If there is disagreement regarding allowing a get_nn_ function to query non-nn
 features, I will add the test for type == NN back in.)


| >  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.
| 
Perhaps a shorter name, e.g.    dccp_feat_nn_get()
(since it returns either the current or next value, and there is no other
 accessor function defined).



| >  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.
|
That is a good point, maybe I need to reread the patch again. From what I saw,
dccp_nn_get_next_val() is so far only called immediately before changing the
value, whereas the above comment hints more at a peek/read-only functionality.

This understanding was also the basis of the attached patch - actually I was
quite happy since its use further simplified the interface.

I think once the above is resolved (which also includes the point below), then
there is quite a good milestone for ccid-2.

| 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.
| 
Agree, having done some testing over the weekend I found that some kind of
hysteresis would be desirable.

I don't know if blocking the Ack Ratio code is the reason, but during the
initial slow-start there were ChangeL requests for Sequence Window on nearly
every packet, which seems too much.

If low_threshold == high_threshold, it oscillates. I think you have already done
some work on this in the code using CCID2_WIN_CHANGE_FACTOR.

During steady state of the connection I did not see many ChangeL requests. 

[-- Attachment #2: simplify_dynamic_nn_negotation.diff --]
[-- Type: text/x-diff, Size: 1964 bytes --]

dccp ccid-2: use new interface to refactor dynamic negotiation

Using the new function dccp_feat_get_nn_next_val(), this simplifies existing
code to not trigger a new negotiation if the requested new value equals the
old one.
---
 net/dccp/ccids/ccid2.c |   22 +++++-----------------
 net/dccp/feat.c        |    5 +++--
 2 files changed, 8 insertions(+), 19 deletions(-)

--- a/net/dccp/feat.c
+++ b/net/dccp/feat.c
@@ -815,10 +815,11 @@ int dccp_feat_signal_nn_change(struct so
 	    !dccp_feat_is_valid_nn_val(feat, nn_val))
 		return -EINVAL;
 
+	if (nn_val == dccp_feat_get_nn_next_val(sk, feat))
+		return 0;	/* already set or negotiation under way */
+
 	entry = dccp_feat_list_lookup(fn, feat, 1);
 	if (entry != NULL) {
-		if (entry->val.nn == fval.nn)
-			return 0;
 		dccp_pr_debug("Clobbering existing NN entry %llu -> %llu\n",
 			      (unsigned long long)entry->val.nn,
 			      (unsigned long long)nn_val);
--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -102,27 +102,15 @@ static void ccid2_change_l_ack_ratio(str
 		DCCP_WARN("Limiting Ack Ratio (%u) to %u\n", val, max_ratio);
 		val = max_ratio;
 	}
-	if (val > DCCPF_ACK_RATIO_MAX)
-		val = DCCPF_ACK_RATIO_MAX;
-
-	if (val == ccid2_ack_ratio_next(sk))
-		return;
-
-	ccid2_pr_debug("changing local ack ratio to %u\n", val);
-	dccp_feat_signal_nn_change(sk, DCCPF_ACK_RATIO, val);
+	dccp_feat_signal_nn_change(sk, DCCPF_ACK_RATIO,
+				   min_t(u32, val, DCCPF_ACK_RATIO_MAX));
 }
 
 static void ccid2_change_l_seq_window(struct sock *sk, u64 val)
 {
-	struct dccp_sock *dp = dccp_sk(sk);
-
-	if (val < DCCPF_SEQ_WMIN)
-		val = DCCPF_SEQ_WMIN;
-	if (val > DCCPF_SEQ_WMAX)
-		val = DCCPF_SEQ_WMAX;
-
-	if (val != dp->dccps_l_seq_win)
-		dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW, val);
+	dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW,
+				   clamp_val(val, DCCPF_SEQ_WMIN,
+						  DCCPF_SEQ_WMAX));
 }
 
 static void ccid2_hc_tx_rto_expire(unsigned long data)

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

[Samuel Jero]

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

> I revised this some more, the function remains the same, but it is shorter 
> http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=commitdiff;h=94da7d70003e8185fe189146f336db72c8fa1f0b
> 
> (If there is disagreement regarding allowing a get_nn_ function to query non-nn
>  features, I will add the test for type == NN back in.)

I think the test for NN features should be added back in. If we allow
the function to be called for non-NN features, we need to include more
code to return the correct values for non-NN features. In either the
negotiating or non-negating state, the current code will return bad
values for non-NN features.

Further, since non-NN features are indeterminate until we receive the
confirm, I don't see that a get_ function for those features would ever
be useful.


> | >  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.
> | 
> Perhaps a shorter name, e.g.    dccp_feat_nn_get()
> (since it returns either the current or next value, and there is no other
>  accessor function defined).

A shorter name is probably a good idea. dccp_feat_nn_get() seems fine.


> | >  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.
> |
> That is a good point, maybe I need to reread the patch again. From what I saw,
> dccp_nn_get_next_val() is so far only called immediately before changing the
> value, whereas the above comment hints more at a peek/read-only functionality.
> 
> This understanding was also the basis of the attached patch - actually I was
> quite happy since its use further simplified the interface.

The patch you attached looks fine.

The additional calls to dccp_nn_get_next_val() are made just before we
call ccid2_change_l_ack_ratio() when we have a loss, idle period, or
application limited period. 

These are the sections of code that you recommended become a separate
patch. I have re-factored that code slightly to reduce code duplication
and will send out that patch immediately after this email.


> | 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.
> | 
> Agree, having done some testing over the weekend I found that some kind of
> hysteresis would be desirable.
> 
> I don't know if blocking the Ack Ratio code is the reason, but during the
> initial slow-start there were ChangeL requests for Sequence Window on nearly
> every packet, which seems too much.

Right now, Change's are sent on every packet until the matching Confirm
is received. That will be the cause of most of the Change's you are
seeing.

The other thing is that at the beginning of a connection the sequence
window will be reduced and then increased. The initial window of 100
packets is too wide for 5*cwnd at the start of the connection. So the
window will be reduced as low as it will go at the very beginning of the
connection. However, as slow start picks up, cwnd will increase and the
sequence window will as well.

I've seen the above behavior, but haven't thought of an effective
solution (nor am I sure that we need a solution). Capping the minimum
sequence window at the default (100 packets) would solve this, but that
doesn't seem like a great solution.

> 
> If low_threshold == high_threshold, it oscillates. I think you have already done
> some work on this in the code using CCID2_WIN_CHANGE_FACTOR.

I'm not entirely certain what low_threshold and high_threshold you are
talking about. I have not seen sequence window oscillations other as the
congestion window oscillates in congestion avoidance as is expected.

Samuel Jero
Internetworking Research Group
Ohio University

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

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

[Gerrit Renker]

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

I have revised the patch set addressing the following comments of this thread:

 1) Renamed function to dccp_feat_nn_get().
 2) Added test to ensure that function is called for NN
    features back in.
 3) Replaced ccid2_ack_ratio_next() with calls to that
    function (dccp_feat_nn_get(sk, DCCPF_ACK_RATIO)).
 4) Replaced check for dccps_l_seq_win in ccid2_change_l_seq_window 
    with call to dccp_feat_nn_get(sk, DCCPF_SEQUENCE_WINDOW)
    (for similar calls see separate, attached patch).
 5) De-inlined ccid2_ack_ratio_next() wrapper as discussed.
 6) Updated remaining patches with regard to 1-4.
 
In (4) I created a separate patch. You mentioned earlier replacing references to
local seq_window with a call to the accessor function, these were the remaining
references, I noticed no regression.

This is all in the test tree, just uploaded 2.6.38
     git://eden-feed.erg.abdn.ac.uk/dccp_exp	[subtree 'dccp']

| > I don't know if blocking the Ack Ratio code is the reason, but during the
| > initial slow-start there were ChangeL requests for Sequence Window on nearly
| > every packet, which seems too much.
| 
| Right now, Change's are sent on every packet until the matching Confirm
| is received. That will be the cause of most of the Change's you are
| seeing.
| 
| The other thing is that at the beginning of a connection the sequence
| window will be reduced and then increased. The initial window of 100
| packets is too wide for 5*cwnd at the start of the connection. So the
| window will be reduced as low as it will go at the very beginning of the
| connection. However, as slow start picks up, cwnd will increase and the
| sequence window will as well.
| 
| I've seen the above behavior, but haven't thought of an effective
| solution (nor am I sure that we need a solution). Capping the minimum
| sequence window at the default (100 packets) would solve this, but that
| doesn't seem like a great solution.
| 
| > 
| > If low_threshold == high_threshold, it oscillates. I think you have already done
| > some work on this in the code using CCID2_WIN_CHANGE_FACTOR.
| 
| I'm not entirely certain what low_threshold and high_threshold you are
| talking about. I have not seen sequence window oscillations other as the
| congestion window oscillates in congestion avoidance as is expected.
|
I meant a Schmitt Trigger like behaviour:
 * low_threshold: to change from lower value to higher value 
 * hi_threshold:  to revert from higher value back to lower value
In these electronic devices there is a gap between low and hi, to avoid fluctuations.
Also audio effect gates have a similar setting.

With regard to your comment about the 100 packets at the begin, I was wondering if
the connection could not grossly exaggerate, or maybe even use the 100 packets.

But all that is not a big deal, stuff for fine-tuning the now much better ccid2 
engine. 

Thank you for all your good work. The comments helped a great deal too.

[-- Attachment #2: remaining_l_seq_win_refs.diff --]
[-- Type: text/x-diff, Size: 1812 bytes --]

dccp ccid-2: replace remaining references to local Sequence Window value

This replaces the remaining references to dccps_l_seq_win with the corresponding
call to retrieve the current (STABLE if not in negotiation, CHANGING if being
negotiated) value of the feature-local sequence window.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ccids/ccid2.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -441,10 +441,10 @@ static void ccid2_new_ack(struct sock *s
 {
 	struct ccid2_hc_tx_sock *hc = ccid2_hc_tx_sk(sk);
 	struct dccp_sock *dp = dccp_sk(sk);
+	u64 l_seq_win = dccp_feat_nn_get(sk, DCCPF_SEQUENCE_WINDOW);
 	int r_seq_used = hc->tx_cwnd / dp->dccps_l_ack_ratio;
 
-	if (hc->tx_cwnd < dp->dccps_l_seq_win &&
-	    r_seq_used < dp->dccps_r_seq_win) {
+	if (hc->tx_cwnd < l_seq_win && r_seq_used < dp->dccps_r_seq_win) {
 		if (hc->tx_cwnd < hc->tx_ssthresh) {
 			if (*maxincr > 0 && ++hc->tx_packets_acked >= 2) {
 				hc->tx_cwnd += 1;
@@ -466,10 +466,10 @@ static void ccid2_new_ack(struct sock *s
 	else if (r_seq_used * CCID2_WIN_CHANGE_FACTOR < dp->dccps_r_seq_win/2)
 		ccid2_change_l_ack_ratio(sk, dp->dccps_l_ack_ratio / 2 ? : 1U);
 
-	if (hc->tx_cwnd * CCID2_WIN_CHANGE_FACTOR >= dp->dccps_l_seq_win)
-		ccid2_change_l_seq_window(sk, dp->dccps_l_seq_win * 2);
-	else if (hc->tx_cwnd * CCID2_WIN_CHANGE_FACTOR < dp->dccps_l_seq_win/2)
-		ccid2_change_l_seq_window(sk, dp->dccps_l_seq_win / 2);
+	if (hc->tx_cwnd * CCID2_WIN_CHANGE_FACTOR >= l_seq_win)
+		ccid2_change_l_seq_window(sk, l_seq_win * 2);
+	else if (hc->tx_cwnd * CCID2_WIN_CHANGE_FACTOR < l_seq_win / 2)
+		ccid2_change_l_seq_window(sk, l_seq_win / 2);
 
 	/*
 	 * FIXME: RTT is sampled several times per acknowledgment (for each

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

[Samuel Jero]

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

> I have revised the patch set addressing the following comments of this thread:
> 
>  1) Renamed function to dccp_feat_nn_get().
>  2) Added test to ensure that function is called for NN
>     features back in.
>  3) Replaced ccid2_ack_ratio_next() with calls to that
>     function (dccp_feat_nn_get(sk, DCCPF_ACK_RATIO)).
>  4) Replaced check for dccps_l_seq_win in ccid2_change_l_seq_window 
>     with call to dccp_feat_nn_get(sk, DCCPF_SEQUENCE_WINDOW)
>     (for similar calls see separate, attached patch).
>  5) De-inlined ccid2_ack_ratio_next() wrapper as discussed.
>  6) Updated remaining patches with regard to 1-4.

Patch Set looks good except for the very last one ("replace remaining
references to local Sequence Window value"). See below for comments
about that patch.


> | > If low_threshold == high_threshold, it oscillates. I think you have already done
> | > some work on this in the code using CCID2_WIN_CHANGE_FACTOR.
> | 
> | I'm not entirely certain what low_threshold and high_threshold you are
> | talking about. I have not seen sequence window oscillations other as the
> | congestion window oscillates in congestion avoidance as is expected.
> |
> I meant a Schmitt Trigger like behaviour:
>  * low_threshold: to change from lower value to higher value 
>  * hi_threshold:  to revert from higher value back to lower value
> In these electronic devices there is a gap between low and hi, to avoid fluctuations.
> Also audio effect gates have a similar setting.

That makes sense now. Thanks for the explanation. I didn't write the
current code with that in mind. However, it does a good job at avoiding
oscillation most of the time.


Below is the very last patch you added to the test tree. I believe both
of the changes made are not needed. See comments inline.
> dccp ccid-2: replace remaining references to local Sequence Window value
> 
> This replaces the remaining references to dccps_l_seq_win with the corresponding
> call to retrieve the current (STABLE if not in negotiation, CHANGING if being
> negotiated) value of the feature-local sequence window.
> 
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> ---
>  net/dccp/ccids/ccid2.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> --- a/net/dccp/ccids/ccid2.c
> +++ b/net/dccp/ccids/ccid2.c
> @@ -441,10 +441,10 @@ static void ccid2_new_ack(struct sock *s
>  {
>  	struct ccid2_hc_tx_sock *hc = ccid2_hc_tx_sk(sk);
>  	struct dccp_sock *dp = dccp_sk(sk);
> +	u64 l_seq_win = dccp_feat_nn_get(sk, DCCPF_SEQUENCE_WINDOW);
>  	int r_seq_used = hc->tx_cwnd / dp->dccps_l_ack_ratio;
>  
> -	if (hc->tx_cwnd < dp->dccps_l_seq_win &&
> -	    r_seq_used < dp->dccps_r_seq_win) {
> +	if (hc->tx_cwnd < l_seq_win && r_seq_used < dp->dccps_r_seq_win) {
>  		if (hc->tx_cwnd < hc->tx_ssthresh) {
>  			if (*maxincr > 0 && ++hc->tx_packets_acked >= 2) {
>  				hc->tx_cwnd += 1;

In this case I believe we want to wait and use the confirmed sequence
window size (like the unmodified code does).

Using the currently negotiating value could result in a congestion
window much larger than the current sequence window (particularly if the
change below is used). This could cause the validity checking code
(dccp_check_seqno) to reject received packets (prior to the confirm) as
sequence invalid. Further, we essentially move back to changing the
effective sequence window before sending the confirm.


> @@ -466,10 +466,10 @@ static void ccid2_new_ack(struct sock *s
>  	else if (r_seq_used * CCID2_WIN_CHANGE_FACTOR < dp->dccps_r_seq_win/2)
>  		ccid2_change_l_ack_ratio(sk, dp->dccps_l_ack_ratio / 2 ? : 1U);
>  
> -	if (hc->tx_cwnd * CCID2_WIN_CHANGE_FACTOR >= dp->dccps_l_seq_win)
> -		ccid2_change_l_seq_window(sk, dp->dccps_l_seq_win * 2);
> -	else if (hc->tx_cwnd * CCID2_WIN_CHANGE_FACTOR < dp->dccps_l_seq_win/2)
> -		ccid2_change_l_seq_window(sk, dp->dccps_l_seq_win / 2);
> +	if (hc->tx_cwnd * CCID2_WIN_CHANGE_FACTOR >= l_seq_win)
> +		ccid2_change_l_seq_window(sk, l_seq_win * 2);
> +	else if (hc->tx_cwnd * CCID2_WIN_CHANGE_FACTOR < l_seq_win / 2)
> +		ccid2_change_l_seq_window(sk, l_seq_win / 2);
>  
>  	/*
>  	 * FIXME: RTT is sampled several times per acknowledgment (for each

Here again, I believe we want to wait and use the confirmed sequence
window size (like the unmodified code does).

Using the currently negotiating value could result in never increasing
the actual sequence window---We ignore confirms for old values so if
begin negotiating a new value before we confirm the previous value we
run the risk of attempting to negotiate a new value several times an
RTT, which results in always receiving (and ignoring) old values.


Samuel Jero
Internetworking Research Group
Ohio University


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Gerrit Renker]

| Below is the very last patch you added to the test tree. I believe both
| of the changes made are not needed. See comments inline.
| > dccp ccid-2: replace remaining references to local Sequence Window value
| > 
Thank you for the explanation. I have removed this patch from the test tree
and resynchronized.

Re: dccp test-tree [RFC] [Patch 2/2] dccp: CCID2 check ack ratio when reducing cwnd

[Samuel Jero]

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

This patch causes CCID2 to check the ack ratio after reducing the
congestion window. If the ack ratio is greater than the congestion
window, it is reduced. This prevents timeouts caused by an ack ratio
larger than the congestion window. 

In this situation, we choose to set the ack ratio to half the congestion
window (or one if that's zero) so that if we loose one ack we don't
trigger a timeout.

--- 
Signed-off-by: Samuel Jero
diff --git a/net/dccp/ccids/ccid2.c b/net/dccp/ccids/ccid2.c
index 1475ba3..b7f7dbd 100644
--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -118,4 +118,22 @@ static void ccid2_change_l_seq_window(struct sock *sk, u64 val)
 		dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW, val);
 }
+
+static void ccid2_check_l_ack_ratio(struct sock *sk)
+{
+	struct ccid2_hc_tx_sock *hc = ccid2_hc_tx_sk(sk);
+
+	/*
+	* After a loss, idle period, application limited period, or RTO we
+	* need tocheck that the ack ratio is still less than the congestion
+	* window. Otherwise, we will send an entire congestion window of
+	* packets and got no response because we haven't sent ack ratio
+	* packets yet.
+	* If the ack ratio does need to be reduced, we reduce it to half of
+	* the congestion window (or 1 if that's zero) instead of to the
+	* congestion window. This prevents problems if one ack is lost.
+	*/
+	if (dccp_feat_get_nn_next_val(sk, DCCPF_ACK_RATIO) > hc->tx_cwnd)
+		ccid2_change_l_ack_ratio(sk, hc->tx_cwnd/2 ? : 1U);
+}
 
 static void ccid2_hc_tx_rto_expire(unsigned long data)
@@ -203,6 +214,8 @@ static void ccid2_cwnd_application_limited(struct sock *sk, const u32 now)
 	}
 	hc->tx_cwnd_used  = 0;
 	hc->tx_cwnd_stamp = now;
+
+	ccid2_check_l_ack_ratio(sk);
 }
 
 /* This borrows the code of tcp_cwnd_restart() */
@@ -221,6 +234,8 @@ static void ccid2_cwnd_restart(struct sock *sk, const u32 now)
 
 	hc->tx_cwnd_stamp = now;
 	hc->tx_cwnd_used  = 0;
+
+	ccid2_check_l_ack_ratio(sk);
 }
 
 static void ccid2_hc_tx_packet_sent(struct sock *sk, unsigned int len)
@@ -490,9 +505,7 @@ static void ccid2_congestion_event(struct sock *sk, struct ccid2_seq *seqp)
 	hc->tx_cwnd      = hc->tx_cwnd / 2 ? : 1U;
 	hc->tx_ssthresh  = max(hc->tx_cwnd, 2U);
 
-	/* Avoid spurious timeouts resulting from Ack Ratio > cwnd */
-	if (dccp_sk(sk)->dccps_l_ack_ratio > hc->tx_cwnd)
-		ccid2_change_l_ack_ratio(sk, hc->tx_cwnd);
+	ccid2_check_l_ack_ratio(sk);
 }
 
 static int ccid2_hc_tx_parse_options(struct sock *sk, u8 packet_type,


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

Re: dccp test-tree [RFC] [Patch 2/2] dccp: CCID2 check ack ratio when reducing cwnd

[Gerrit Renker]

| This patch causes CCID2 to check the ack ratio after reducing the
| congestion window.
Thanks a lot, it has been integrated with the other changes:
  http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=commitdiff;h=f7bcfc7ddf5c1f61c861c3d2b475df7874833ed3