Quoting Samuel Jero:
| > | My recommendation is to wait until the Confirm has been received to
| > | alter the value of an NN feature. This eliminates the problem above and
| > | has the advantage that it follows RFC 4340 (section 6.6.1). Further,
| > | making this change should require little code change.
| > |
| > I like the solution introduced in patch #3 because it is simple: Change
| > options are retransmitted until the correct Confirm option comes in,
| > instead of resetting the connection. It is an instance of the robustness
| > principle - the peer can send any outdated/reordered Confirm option.
|
| Patches #2 and 3 (and another patch I will send shortly) make the
| current method usable. Without them, NN feature negotiation in an
| ongoing connection causes the connection to be reset.
|
Thank you for sending the updates. I have applied them to the current tree,
please check them. Here are the changes that I made:
Patch #1: only whitespace changes. When looking over this patch I noted a problem
with setting the local sequence window directly: this is also done in
feat.c:dccp_hdlr_seq_win() - in particular, also adjusting GSS, AWL, AWH.
Please see discussion below, this is related.
Patch #4: I think your patch set has tackled some of the problems mentioned
in the FIXME that it removes. It would be good to find out what
remains to be tackled, to eventually remove the #ifdef.
Patch #5: Added a dccp_feat_list_pop(entry) in case the new value overwrites
an existing one. Otherwise there would be multiple ChangeL options
in the same packet, with differing values. Please also see below.
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=shortlog;h=dccp
| However, I wouldn't consider them a fix for what I'm talking about here.
| The fact is that with the current code the sender thinks the receiver
| has the new value of the feature while the feature is being negotiated.
|
I think this was my fault: the RFC says in 6.6.1 that values MUST not be changed
until they are confirmed by the peer. When this is taken as a prerequisite, the
complexity you mentioned below and the weird cases of two ends being in a limbo
state will not appear.
I am therefore sending an RFC patch separately. Although I have tried it briefly
with some Speex-streaming yesterday, I have not yet applied it to the test tree:
could you give this one a brief spin with your test setup?
If it works out as expected, I think this will give a simpler (and more conformant)
solution.
| Particularly for the Ack Ratio this can be confusing because the sender
| would interpret the acks from the receiver using the new ack ratio. The
| first half RTT of those would be from the old ack ratio. This would
| cause inappropriate changes in the congestion window/pipe except for
| appropriate byte counting which, I believe, mitigates that.
|
| It just seems to me that there should be a better way to do this;
| something that's clearer and easier to understand/debug.
|
I wonder if there are any corner cases with waiting for the ConfirmR option before
activating the case: if not, the case is simpler, and I think we can then even
remove the following (changed) segment.
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);
dccp_feat_list_pop(entry);
}
--
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
