From patchwork Fri Mar 18 11:30:52 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gerrit Renker X-Patchwork-Id: 87514 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 7F915B6FA7 for ; Fri, 18 Mar 2011 22:31:12 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753102Ab1CRLbH (ORCPT ); Fri, 18 Mar 2011 07:31:07 -0400 Received: from dee.erg.abdn.ac.uk ([139.133.204.82]:44901 "EHLO erg.abdn.ac.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752183Ab1CRLbF (ORCPT ); Fri, 18 Mar 2011 07:31:05 -0400 Received: from laptev.erg.abdn.ac.uk (Debian-exim@ra-gerrit.erg.abdn.ac.uk [139.133.204.38]) by erg.abdn.ac.uk (8.13.4/8.13.4) with ESMTP id p2IBUqat005812 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NOT); Fri, 18 Mar 2011 11:30:53 GMT Received: from gerrit by laptev.erg.abdn.ac.uk with local (Exim 4.69) (envelope-from ) id 1Q0XtM-0001Rk-Jq; Fri, 18 Mar 2011 12:30:52 +0100 Date: Fri, 18 Mar 2011 12:30:52 +0100 From: Gerrit Renker To: Samuel Jero Cc: dccp@vger.kernel.org, netdev@vger.kernel.org Subject: Re: dccp test-tree [RFC] [Patch 1/1] dccp: Only activate NN values after receiving the Confirm option Message-ID: <20110318113052.GA5508@gerrit.erg.abdn.ac.uk> Mail-Followup-To: Gerrit Renker , Samuel Jero , dccp@vger.kernel.org, netdev@vger.kernel.org References: <20110228112511.GE3620@gerrit.erg.abdn.ac.uk> <1299559831.7569.41.camel@jero-laptop> <20110311113042.GB4876@gerrit.erg.abdn.ac.uk> <1300048497.31664.158.camel@jero-laptop> <20110314115501.GB4631@gerrit.erg.abdn.ac.uk> <1300164791.20638.64.camel@jero-laptop> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1300164791.20638.64.camel@jero-laptop> User-Agent: Mutt/1.5.18 (2008-05-17) X-ERG-MailScanner: Found to be clean X-ERG-MailScanner-From: gerrit@erg.abdn.ac.uk X-Spam-Status: No Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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. 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 --- 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