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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110314115501.GB4631@gerrit.erg.abdn.ac.uk>
Date:	Mon, 14 Mar 2011 12:55:01 +0100
From:	Gerrit Renker <gerrit@....abdn.ac.uk>
To:	Samuel Jero <sj323707@...o.edu>
Cc:	dccp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: dccp test-tree [RFC] [Patch 1/1] dccp: Only activate NN values
	after receiving the Confirm option

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. 

View attachment "simplify_dynamic_nn_negotation.diff" of type "text/x-diff" (1965 bytes)