Message ID | 1299559831.7569.41.camel@jero-laptop |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
| 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 > > | 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. Separate patch coming shortly. Will add comment describing the situation. > 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? 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. > 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); That should also be updated to use dccp_feat_get_nn_next_val(sk, DCCPF_SEQUENCE_WINDOW) > 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. 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. > 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). I'll create a function for this code. Coming in 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. Go ahead with 1) and 2). I'll send out a new patch for 3 (b) shortly. Samuel Jero Internetworking Research Group Ohio University
diff --git a/net/dccp/ccids/ccid2.c b/net/dccp/ccids/ccid2.c --- a/net/dccp/ccids/ccid2.c +++ b/net/dccp/ccids/ccid2.c @@ -83,9 +82,13 @@ static int ccid2_hc_tx_send_packet(struct sock *sk, struct sk_buff *skb) return CCID_PACKET_SEND_AT_ONCE; } +static __u64 ccid2_ack_ratio_next(struct sock *sk) +{ + return dccp_feat_get_nn_next_val(sk, DCCPF_ACK_RATIO); +} + static void ccid2_change_l_ack_ratio(struct sock *sk, u32 val) { - struct dccp_sock *dp = dccp_sk(sk); u32 max_ratio = DIV_ROUND_UP(ccid2_hc_tx_sk(sk)->tx_cwnd, 2); /* @@ -101,11 +104,10 @@ static void ccid2_change_l_ack_ratio(struct sock *sk, u32 val) if (val > DCCPF_ACK_RATIO_MAX) val = DCCPF_ACK_RATIO_MAX; - if (val == dp->dccps_l_ack_ratio) + if (val == ccid2_ack_ratio_next(sk)) return; ccid2_pr_debug("changing local ack ratio to %u\n", val); - dp->dccps_l_ack_ratio = val; dccp_feat_signal_nn_change(sk, DCCPF_ACK_RATIO, val); } @@ -117,11 +119,9 @@ static void ccid2_change_l_seq_window(struct sock *sk, u64 val) val = DCCPF_SEQ_WMIN; if (val > DCCPF_SEQ_WMAX) val = DCCPF_SEQ_WMAX; - if (val == dp->dccps_l_seq_win) - return; - dp->dccps_l_seq_win = val; - dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW, val); + if (val != dp->dccps_l_seq_win) + dccp_feat_signal_nn_change(sk, DCCPF_SEQUENCE_WINDOW, val); } static void ccid2_hc_tx_rto_expire(unsigned long data) @@ -203,6 +203,10 @@ static void ccid2_cwnd_application_limited(struct sock *sk, const u32 now) } hc->tx_cwnd_used = 0; hc->tx_cwnd_stamp = now; + + /* Avoid spurious timeouts resulting from Ack Ratio > cwnd */ + if (ccid2_ack_ratio_next(sk) > hc->tx_cwnd) + ccid2_change_l_ack_ratio(sk, hc->tx_cwnd/2 ? : 1U); } /* This borrows the code of tcp_cwnd_restart() */ @@ -221,6 +225,10 @@ static void ccid2_cwnd_restart(struct sock *sk, const u32 now) hc->tx_cwnd_stamp = now; hc->tx_cwnd_used = 0; + + /* Avoid spurious timeouts resulting from Ack Ratio > cwnd */ + if (ccid2_ack_ratio_next(sk) > hc->tx_cwnd) + ccid2_change_l_ack_ratio(sk, hc->tx_cwnd/2 ? : 1U); } static void ccid2_hc_tx_packet_sent(struct sock *sk, unsigned int len) @@ -491,8 +531,8 @@ static void ccid2_congestion_event(struct sock *sk, struct ccid2_seq *seqp) hc->tx_ssthresh = max(hc->tx_cwnd, 2U); /* Avoid spurious timeouts resulting from Ack Ratio > cwnd */ - if (dccp_sk(sk)->dccps_l_ack_ratio > hc->tx_cwnd) - ccid2_change_l_ack_ratio(sk, hc->tx_cwnd); + if (ccid2_ack_ratio_next(sk) > hc->tx_cwnd) + ccid2_change_l_ack_ratio(sk, hc->tx_cwnd/2 ? : 1U); } static int ccid2_hc_tx_parse_options(struct sock *sk, u8 packet_type, diff --git a/net/dccp/dccp.h b/net/dccp/dccp.h index 26cba68..e8cf5a7 100644 --- a/net/dccp/dccp.h +++ b/net/dccp/dccp.h @@ -491,6 +491,7 @@ static inline int dccp_ack_pending(const struct sock *sk) } extern int dccp_feat_signal_nn_change(struct sock *sk, u8 feat, u64 nn_val); +extern __u64 dccp_feat_get_nn_next_val(struct sock *sk, u8 feat); extern int dccp_feat_finalise_settings(struct dccp_sock *dp); extern int dccp_feat_server_ccid_dependencies(struct dccp_request_sock *dreq); extern int dccp_feat_insert_opts(struct dccp_sock*, struct dccp_request_sock*, diff --git a/net/dccp/feat.c b/net/dccp/feat.c index 9a8c2ba..bc26f33 100644 --- a/net/dccp/feat.c +++ b/net/dccp/feat.c @@ -775,12 +775,7 @@ int dccp_feat_register_sp(struct sock *sk, u8 feat, u8 is_local, * @sk: DCCP socket of an established connection * @feat: NN feature number from %dccp_feature_numbers * @nn_val: the new value to use - * This function is used to communicate NN updates out-of-band. The difference - * to feature negotiation during connection setup is that values are activated - * immediately after validation, i.e. we don't wait for the Confirm: either the - * value is accepted by the peer (and then the waiting is futile), or it is not - * (Reset or empty Confirm). We don't accept empty Confirms - transmitted values - * are validated, and the peer "MUST accept any valid value" (RFC 4340, 6.3.2). + * This function is used to communicate NN updates out-of-band. */ int dccp_feat_signal_nn_change(struct sock *sk, u8 feat, u64 nn_val) { @@ -805,9 +800,6 @@ int dccp_feat_signal_nn_change(struct sock *sk, u8 feat, u64 nn_val) dccp_feat_list_pop(entry); } - if (dccp_feat_activate(sk, feat, 1, &fval)) - return -EADV; - inet_csk_schedule_ack(sk); return dccp_feat_push_change(fn, feat, 1, 0, &fval); } @@ -1356,6 +1348,9 @@ static u8 dccp_feat_handle_nn_established(struct sock *sk, u8 mandatory, u8 opt, if (fval.nn != entry->val.nn) return 0; + /* Only activate after receiving the Confirm option (6.6.1). */ + dccp_feat_activate(sk, feat, local, &fval); + /* It has been confirmed - so remove the entry */ dccp_feat_list_pop(entry); @@ -1374,6 +1369,39 @@ fast_path_failed: : DCCP_RESET_CODE_OPTION_ERROR; } +/* +* dccp_feat_get_nn_next_val - Get the currently negotiating value of an NN +* feature. If the feature isn't being currently +* negotiated, return it's current value. +* @sk: DCCP socket of an established connection +* @feat: NN feature number from %dccp_feature_numbers +*/ +__u64 dccp_feat_get_nn_next_val(struct sock *sk, u8 feat) +{ + struct list_head *fn = &dccp_sk(sk)->dccps_featneg; + struct dccp_feat_entry *entry; + u8 type = dccp_feat_type(feat); + + if (type == FEAT_UNKNOWN || type != FEAT_NN) + return 0; + + entry = dccp_feat_list_lookup(fn, feat, 1); + if (entry != NULL) + return entry->val.nn; + + switch (feat) { + case DCCPF_ACK_RATIO: + return dccp_sk(sk)->dccps_l_ack_ratio; + case DCCPF_SEQUENCE_WINDOW: + return dccp_sk(sk)->dccps_l_seq_win; + default: + dccp_pr_debug("Attempting to get value \ + for unknown NN feature"); + } +return 0; +} +EXPORT_SYMBOL_GPL(dccp_feat_get_nn_next_val); + /** * dccp_feat_parse_options - Process Feature-Negotiation Options * @sk: for general use and used by the client during connection setup