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]

Date:	Fri, 11 Mar 2011 12:30:42 +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

| 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html