From patchwork Mon Mar 14 11:55:01 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gerrit Renker X-Patchwork-Id: 86723 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 47B47B6F72 for ; Mon, 14 Mar 2011 22:55:18 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754762Ab1CNLzN (ORCPT ); Mon, 14 Mar 2011 07:55:13 -0400 Received: from dee.erg.abdn.ac.uk ([139.133.204.82]:61269 "EHLO erg.abdn.ac.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754365Ab1CNLzM (ORCPT ); Mon, 14 Mar 2011 07:55:12 -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 p2EBt2Ck019880 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NOT); Mon, 14 Mar 2011 11:55:02 GMT Received: from gerrit by laptev.erg.abdn.ac.uk with local (Exim 4.69) (envelope-from ) id 1Pz6MX-0001Yp-Qv; Mon, 14 Mar 2011 12:55:01 +0100 Date: Mon, 14 Mar 2011 12:55:01 +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: <20110314115501.GB4631@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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1300048497.31664.158.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 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. 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)