diff mbox

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

Message ID 20110314115501.GB4631@gerrit.erg.abdn.ac.uk
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Gerrit Renker March 14, 2011, 11:55 a.m. UTC 
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.

Comments

Samuel Jero March 15, 2011, 4:53 a.m. UTC | #1 
> 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
diff mbox

Patch

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)