: Bug-Fixes and Clean-Ups
Subsystems (4)
A message that is not in the archive (sent off-list or not yet received): bug_fixes_and_cleanups
cover : Bug-Fixes and Clean-Ups
This set contains bug-fixes and clean-ups. The first two are already in the test tree:
Patch #1: Fixes a bug where CCID-3 tries to sample the RTT while the
history is empty.
Leandro - I have added the same bug fix in the CCID-4 tree, just after patch #24.
Patch #2: Deprecates the Ack Ratio sysctl as per earlier discussion.
The following are kept back for the moment, since the first patch is an RFC patch, and since a lot of the CCID-4 code depends on this:
Patch #3: Continues for consistency the earlier patch which removed the
long prefixes for CCID-3.
Arnaldo - can you please check. Also Leandro, can you also check this - it has implications for CCID-4, too; which is why I have not put it into the test tree immediately.
Patch #4: Fixes two sparse warnings.
Patch #5: Found out that RFC3390 code is identically applied in three
places (TCP, CCID-2, and CCID-3): this consolidates the duplicated code.
Note: there is a fourth place where this code is duplicated
(CCID-4), but this needs to be patched separately.
Patch #6: Tidies up the slow-path input processing for DCCP (i.e. input
for states other than OPEN). This addresses some reordering quirks by putting steps in their RFC4340-like order, ensures that CCID/Ack Vector input is indeed confined to the fast-path, and contains one bug fix.
Patch #7: Removes dead states, thanks to an observation by Leandro and
his colleagues.
Note: This would also need to be ported to CCID-4, since it
uses the same (dead) states.
If no one shouts, I will put that into the tree approximately by the end of the week. In any case it would be good to know if the CCID-4 tree is still active, since porting these changes will take quite a bit of work.
In particular - patch #5 may be ported to CCID-4 as well, but it depends on whether people find the code consolidation acceptable.
The first two patches can be obtained in the sets from
(1) DCCP git pull dccp://eden-feed.erg.abdn.ac.uk/dccp_exp +dccp:my_dccp
(2) CCID-4 git pull dccp://eden-feed.erg.abdn.ac.uk/dccp_exp +ccid4:my_ccid4
PATCH 1/7 : Don't interpret feedback when nothing has been sent
This is a BUG-Fix for CCID-3 which, on a server, lead to such messages:
ccid3_hc_tx_packet_recv: server(e7b7d518): DATAACK with bogus ACK-281971487
ccid3_hc_tx_packet_recv: server(e7b7d518): DATAACK with bogus ACK-2664160693
ccid3_hc_tx_packet_recv: server(e7b7d030): DATAACK with bogus ACK-2775299822
ccid3_hc_tx_packet_recv: server(e7b7d518): DATAACK with bogus ACK-3035830110
ccid3_hc_tx_packet_recv: server(e7b7d030): DATAACK with bogus ACK-3436194561
The cause was that the (passive) server hadn't sent anything, i.e. its TX history was empty. This patch adds the missing test for an empty TX history, i.e. packets carrying Ack information are ignored if nothing has been sent.
I wanted to keep this generic, i.e. use a "hist_empty()" function instead of "hctx->hist = NULL". Also replaced multiple occurrences of DCCP_SKB_CB.
--- net/dccp/ccids/ccid3.c | 16 +++++++++------- net/dccp/ccids/lib/packet_history.h | 4 ++++ 2 files changed, 13 insertions(+), 7 deletions(-)
net/dccp/ccids/ccid3.c +9 -7
@@ -384,13 +384,15 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb) {struct ccid3_hc_tx_sock *hctx =ccid3_hc_tx_sk (sk);struct ccid3_options_received *opt_recv; +struct dccp_skb_cb *dcb =DCCP_SKB_CB (skb);ktime_t now;unsigned long t_nfb;u32 pinv, r_sample; -/* we are only interested in ACKs */ -if (!(DCCP_SKB_CB (skb)->dccpd_type =DCCP_PKT_ACK || -DCCP_SKB_CB (skb)->dccpd_type =DCCP_PKT_DATAACK )) +/* Ignore if nothing has been sent; feedback is on Ack/DataAck only */ +if (tfrc_tx_hist_is_empty (hctx->ccid3hctx_hist) || + !(dcb->dccpd_type =DCCP_PKT_ACK || + dcb->dccpd_type =DCCP_PKT_DATAACK ))return ;/* ... and only in the established state */ if (hctx->ccid3hctx_state != TFRC_SSTATE_FBACK &&
@@ -402,11 +404,11 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)/* Estimate RTT from history if ACK number is valid */ r_sample =tfrc_tx_hist_rtt (hctx->ccid3hctx_hist, -DCCP_SKB_CB (skb)->dccpd_ack_seq, now); + dcb->dccpd_ack_seq, now);if (r_sample =0 ) { -DCCP_WARN ("%s(%p): %s with bogus ACK-%llu\n" ,dccp_role (sk), sk, -dccp_packet_name (DCCP_SKB_CB (skb)->dccpd_type), - (unsigned long long )DCCP_SKB_CB (skb)->dccpd_ack_seq); +DCCP_WARN ("%s(%p): %s with bogus ACK #%llu\n" , +dccp_role (sk), sk,dccp_packet_name (dcb->dccpd_type), + (unsigned long long )dcb->dccpd_ack_seq);return ; }
net/dccp/ccids/lib/packet_history.h +4 -0
@@ -42,6 +42,10 @@struct tfrc_tx_hist_entry ; +static inline bool tfrc_tx_hist_is_empty (const struct tfrc_tx_hist_entry *head) +{ +return head =NULL ; +}extern int tfrc_tx_hist_add (struct tfrc_tx_hist_entry **headp,u64 seqno);extern void tfrc_tx_hist_purge (struct tfrc_tx_hist_entry **headp); extern u32 tfrc_tx_hist_rtt(struct tfrc_tx_hist_entry *head,
Signature · 1 line
1.5.3.GIT
PATCH 2/7 : Deprecate Ack Ratio sysctl
This patch deprecates the Ack Ratio sysctl because
* Ack Ratio is entirely ignored by CCID-3 and CCID-4,
* Ack Ratio currently doesn't work in CCID-2 (i.e. is always set to 1);
* even if it would work in CCID-2, there is no point for a user to change it:
- Ack Ratio is constrained by cwnd (RFC 4341, 6.1.2),
- if Ack Ratio > cwnd, the system resorts to spurious RTO timeouts (since waiting for Acks which will never arrive in this window),
- cwnd is not a user-configurable value.
The only reasonable place for Ack Ratio is to print it for debugging. It is planned to do this later on, as part of e.g. dccp_probe.
With this patch Ack Ratio is now under full control of feature negotiation:
* Ack Ratio is resolved as a dependency of the selected CCID;
* if the chosen CCID supports it (i.e. CCID = CCID-2), Ack Ratio is set to the default of 2, following RFC 4340, 11.3 - "New connections start with Ack Ratio 2 for both endpoints";
* what happens then is part of another patch set, since it concerns the dynamic update of Ack Ratio while the connection is in full flight.
Thanks to Tomasz Grobelny for discussion leading up to this patch.
--- Documentation/networking/dccp.txt | 3 --- include/linux/dccp.h | 2 -- net/dccp/dccp.h | 1 - net/dccp/minisocks.c | 1 - net/dccp/options.c | 1 - net/dccp/sysctl.c | 7 ------- 6 files changed, 0 insertions(+), 15 deletions(-)
Documentation/networking/dccp.txt +0 -3
@@ -125,9 +125,6 @@ send_ndp = 1 send_ackvec = 1 Whether or not to send Ack Vector options (sec. 11.5). -ack_ratio = 2 - The default Ack Ratio (sec. 11.3) to use. - tx_ccid = 2 Default CCID for the sender-receiver half-connection.
include/linux/dccp.h +0 -2
@@ -370,7 +370,6 @@ static inline unsigned int dccp_hdr_len(const struct sk_buff *skb)* @dccpms_ccid - Congestion Control Id (CCID) (section 10) * @dccpms_send_ack_vector - Send Ack Vector Feature (section 11.5) * @dccpms_send_ndp_count - Send NDP Count Feature (7.7.2) -* @dccpms_ack_ratio - Ack Ratio Feature (section 11.3) * @dccpms_pending - List of features being negotiated * @dccpms_conf - */
@@ -380,7 +379,6 @@ struct dccp_minisock {__u8 dccpms_tx_ccid;__u8 dccpms_send_ack_vector;__u8 dccpms_send_ndp_count; -__u8 dccpms_ack_ratio;struct list_head dccpms_pending;struct list_head dccpms_conf; };
net/dccp/dccp.h +0 -1
@@ -98,7 +98,6 @@ extern int sysctl_dccp_retries2;extern int sysctl_dccp_feat_sequence_window;extern int sysctl_dccp_feat_rx_ccid;extern int sysctl_dccp_feat_tx_ccid; -extern int sysctl_dccp_feat_ack_ratio;extern int sysctl_dccp_feat_send_ack_vector;extern int sysctl_dccp_feat_send_ndp_count;extern int sysctl_dccp_tx_qlen;
net/dccp/minisocks.c +0 -1
@@ -47,7 +47,6 @@ void dccp_minisock_init(struct dccp_minisock *dmsk) dmsk->dccpms_sequence_window = sysctl_dccp_feat_sequence_window; dmsk->dccpms_rx_ccid = sysctl_dccp_feat_rx_ccid; dmsk->dccpms_tx_ccid = sysctl_dccp_feat_tx_ccid; - dmsk->dccpms_ack_ratio = sysctl_dccp_feat_ack_ratio; dmsk->dccpms_send_ack_vector = sysctl_dccp_feat_send_ack_vector; dmsk->dccpms_send_ndp_count = sysctl_dccp_feat_send_ndp_count; }
net/dccp/options.c +0 -1
@@ -26,7 +26,6 @@int sysctl_dccp_feat_sequence_window =DCCPF_INITIAL_SEQUENCE_WINDOW ;int sysctl_dccp_feat_rx_ccid =DCCPF_INITIAL_CCID ;int sysctl_dccp_feat_tx_ccid =DCCPF_INITIAL_CCID ; -int sysctl_dccp_feat_ack_ratio =DCCPF_INITIAL_ACK_RATIO ;int sysctl_dccp_feat_send_ack_vector =DCCPF_INITIAL_SEND_ACK_VECTOR ;int sysctl_dccp_feat_send_ndp_count =DCCPF_INITIAL_SEND_NDP_COUNT ;
net/dccp/sysctl.c +0 -7
@@ -41,13 +41,6 @@ static struct ctl_table dccp_default_table[] = { .proc_handler = proc_dointvec, }, { - .procname = "ack_ratio", - .data = &sysctl_dccp_feat_ack_ratio, - .maxlen = sizeof(sysctl_dccp_feat_ack_ratio), - .mode = 0644, - .proc_handler = proc_dointvec, - }, - { .procname = "send_ackvec", .data = &sysctl_dccp_feat_send_ack_vector, .maxlen = sizeof(sysctl_dccp_feat_send_ack_vector),
Signature · 1 line
1.5.3.GIT
RFC PATCH 3/7 : Get rid of ccid3hc{tx,rx}_ prefixes
Arnaldo can you please check if you are ok with this - it is a follow-up to the earlier patch, for consistency.
--------------------------> Patch <----------------------------------------- This continues work done in an earlier patch for CCID-2, where there was agreement on dccp@vger that the `hctx->ccid3hctx_' prefixes were too long.
Thus this patch does the same for CCID-3 as the previous patch for CCID-2:
s#ccid3hctx_##g; s#ccid3hcrx_##g;
plus manual editing to retain consistency (updated comments, moved assignment within an `if' expression into standalone expression).
As in the previous patch, the code becomes much clearer to read, and many multi-line expressions are reduced in size.
Note: expanded the fields of the `struct tfrc_tx_info' in the hc_tx_sock, since
using short #define identifiers is not a good idea. The only place where this embedded struct was used is ccid3_hc_tx_getsockopt(). (It may be that removing the embedded struct allows to reduce the size of the socket, by re-arranging fields; I haven't checked this.)
--- net/dccp/ccids/ccid3.c | 335 +++++++++++++++++++++++-------------------------- net/dccp/ccids/ccid3.h | 119 ++++++++--------- net/dccp/probe.c | 8 - 3 files changed, 221 insertions(+), 241 deletions(-)
net/dccp/ccids/ccid3.h +59 -60
@@ -77,44 +77,43 @@ enum ccid3_hc_tx_states {/** struct ccid3_hc_tx_sock - CCID3 sender half-connection socket * -* @ccid3hctx_x - Current sending rate in 64 * bytes per second -* @ccid3hctx_x_recv - Receive rate in 64 * bytes per second -* @ccid3hctx_x_calc - Calculated rate in bytes per second -* @ccid3hctx_rtt - Estimate of current round trip time in usecs -* @ccid3hctx_p - Current loss event rate (0-1) scaled by 1000000 -* @ccid3hctx_s - Packet size in bytes -* @ccid3hctx_t_rto - Nofeedback Timer setting in usecs -* @ccid3hctx_t_ipi - Interpacket (send) interval (RFC 3448, 4.6) in usecs -* @ccid3hctx_state - Sender state, one of %ccid3_hc_tx_states -* @ccid3hctx_last_win_count - Last window counter sent -* @ccid3hctx_t_last_win_count - Timestamp of earliest packet -* with last_win_count value sent -* @ccid3hctx_no_feedback_timer - Handle to no feedback timer -* @ccid3hctx_t_ld - Time last doubled during slow start -* @ccid3hctx_t_nom - Nominal send time of next packet -* @ccid3hctx_delta - Send timer delta (RFC 3448, 4.6) in usecs -* @ccid3hctx_hist - Packet history -* @ccid3hctx_options_received - Parsed set of retrieved options +* @x - Current sending rate in 64 * bytes per second +* @x_recv - Receive rate in 64 * bytes per second +* @x_calc - Calculated rate in bytes per second +* @rtt - Estimate of current round trip time in usecs +* @p - Current loss event rate (0-1) scaled by 1000000 +* @s - Packet size in bytes +* @t_rto - Nofeedback Timer setting in usecs +* @t_ipi - Interpacket (send) interval (RFC 3448, 4.6) in usecs +* @state - Sender state, one of %ccid3_hc_tx_states +* @last_win_count - Last window counter sent +* @t_last_win_count - Timestamp of earliest packet with +* last_win_count value sent +* @no_feedback_timer - Handle to no feedback timer +* @t_ld - Time last doubled during slow start +* @t_nom - Nominal send time of next packet +* @delta - Send timer delta (RFC 3448, 4.6) in usecs +* @hist - Packet history +* @options_received - Parsed set of retrieved options */ struct ccid3_hc_tx_sock { -struct tfrc_tx_info ccid3hctx_tfrc; -#define ccid3hctx_x ccid3hctx_tfrc.tfrctx_x -#define ccid3hctx_x_recv ccid3hctx_tfrc.tfrctx_x_recv -#define ccid3hctx_x_calc ccid3hctx_tfrc.tfrctx_x_calc -#define ccid3hctx_rtt ccid3hctx_tfrc.tfrctx_rtt -#define ccid3hctx_p ccid3hctx_tfrc.tfrctx_p -#define ccid3hctx_t_rto ccid3hctx_tfrc.tfrctx_rto -#define ccid3hctx_t_ipi ccid3hctx_tfrc.tfrctx_ipi -u16 ccid3hctx_s; -enum ccid3_hc_tx_states ccid3hctx_state:8 ; -u8 ccid3hctx_last_win_count; -ktime_t ccid3hctx_t_last_win_count; -struct timer_list ccid3hctx_no_feedback_timer; -ktime_t ccid3hctx_t_ld; -ktime_t ccid3hctx_t_nom; -u32 ccid3hctx_delta; -struct tfrc_tx_hist_entry *ccid3hctx_hist; -struct ccid3_options_received ccid3hctx_options_received; +u64 x; +u64 x_recv; +u32 x_calc; +u32 rtt; +u32 p; +u32 t_rto; +u32 t_ipi; +u16 s; +enum ccid3_hc_tx_states state:8 ; +u8 last_win_count; +ktime_t t_last_win_count; +struct timer_list no_feedback_timer; +ktime_t t_ld; +ktime_t t_nom; +u32 delta; +struct tfrc_tx_hist_entry *hist; +struct ccid3_options_received options_received; };static inline struct ccid3_hc_tx_sock *ccid3_hc_tx_sk (const struct sock *sk)
@@ -133,32 +132,32 @@ enum ccid3_hc_rx_states {/** struct ccid3_hc_rx_sock - CCID3 receiver half-connection socket * -* @ccid3hcrx_x_recv - Receiver estimate of send rate (RFC 3448 4.3) -* @ccid3hcrx_rtt - Receiver estimate of rtt (non-standard) -* @ccid3hcrx_p - Current loss event rate (RFC 3448 5.4) -* @ccid3hcrx_last_counter - Tracks window counter (RFC 4342, 8.1) -* @ccid3hcrx_state - Receiver state, one of %ccid3_hc_rx_states -* @ccid3hcrx_bytes_recv - Total sum of DCCP payload bytes -* @ccid3hcrx_x_recv - Receiver estimate of send rate (RFC 3448, sec. 4.3) -* @ccid3hcrx_rtt - Receiver estimate of RTT -* @ccid3hcrx_tstamp_last_feedback - Time at which last feedback was sent -* @ccid3hcrx_tstamp_last_ack - Time at which last feedback was sent -* @ccid3hcrx_hist - Packet history (loss detection + RTT sampling) -* @ccid3hcrx_li_hist - Loss Interval database -* @ccid3hcrx_s - Received packet size in bytes -* @ccid3hcrx_pinv - Inverse of Loss Event Rate (RFC 4342, sec. 8.5) +* @x_recv - Receiver estimate of send rate (RFC 3448 4.3) +* @rtt - Receiver estimate of rtt (non-standard) +* @p - Current loss event rate (RFC 3448 5.4) +* @last_counter - Tracks window counter (RFC 4342, 8.1) +* @state - Receiver state, one of %ccid3_hc_rx_states +* @bytes_recv - Total sum of DCCP payload bytes +* @x_recv - Receiver estimate of send rate (RFC 3448, sec. 4.3) +* @rtt - Receiver estimate of RTT +* @tstamp_last_feedback - Time at which last feedback was sent +* @tstamp_last_ack - Time at which last feedback was sent +* @hist - Packet history (loss detection + RTT sampling) +* @li_hist - Loss Interval database +* @s - Received packet size in bytes +* @p_inverse - Inverse of Loss Event Rate (RFC 4342, sec. 8.5) */ struct ccid3_hc_rx_sock { -u8 ccid3hcrx_last_counter:4 ; -enum ccid3_hc_rx_states ccid3hcrx_state:8 ; -u32 ccid3hcrx_bytes_recv; -u32 ccid3hcrx_x_recv; -u32 ccid3hcrx_rtt; -ktime_t ccid3hcrx_tstamp_last_feedback; -struct tfrc_rx_hist ccid3hcrx_hist; -struct tfrc_loss_hist ccid3hcrx_li_hist; -u16 ccid3hcrx_s; -#define ccid3hcrx_pinv ccid3hcrx_li_hist.i_mean +u8 last_counter:4 ; +enum ccid3_hc_rx_states state:8 ; +u32 bytes_recv; +u32 x_recv; +u32 rtt; +ktime_t tstamp_last_feedback; +struct tfrc_rx_hist hist; +struct tfrc_loss_hist li_hist; +u16 s; +#define p_inverse li_hist.i_mean };static inline struct ccid3_hc_rx_sock *ccid3_hc_rx_sk (const struct sock *sk)
net/dccp/ccids/ccid3.c +159 -172
725 lines not on this page · raw message
net/dccp/probe.c +3 -5
@@ -71,11 +71,9 @@ static void jdccp_write_xmit(struct sockif (hctx) len +=sprintf (tbuf + len," %d %d %d %u %u %u %d" , - hctx->ccid3hctx_s, hctx->ccid3hctx_rtt, - hctx->ccid3hctx_p, hctx->ccid3hctx_x_calc, - (unsigned )(hctx->ccid3hctx_x_recv >>6 ), - (unsigned )(hctx->ccid3hctx_x >>6 ), - hctx->ccid3hctx_t_ipi); + hctx->s, hctx->rtt, hctx->p, hctx->x_calc, + (unsigned )(hctx->x_recv >>6 ), + (unsigned )(hctx->x >>6 ), hctx->t_ipi); len +=sprintf (tbuf + len,"\n" );kfifo_put (dccpw.fifo, tbuf, len);
PATCH 4/7 : Fix sparse warnings
This patch fixes the following two sparse warnings:
1. Due to a nested min(max()) expression, sparse gave the following warnings:
net/dccp/ccids/ccid3.c:91:21: warning: symbol '__x' shadows an earlier one net/dccp/ccids/ccid3.c:91:21: warning: symbol '__y' shadows an earlier one
In addition, in update_x(), the function call to scaled_div() appeared as macro parameter and was thus evaluated multiple times.
Fixed by disentangling the nested expression - code also clearer now.
2. Declaration of function prototypes was in .c instead of .h file. This resulted in "should it be static" warnings. Corrected by moving the declarations to the tfrc.h file instead of the tfrc.c file.
--- net/dccp/ccids/ccid3.c | 11 +++++++---- net/dccp/ccids/lib/tfrc.c | 8 -------- net/dccp/ccids/lib/tfrc.h | 11 +++++++++-- 3 files changed, 16 insertions(+), 14 deletions(-)
net/dccp/ccids/ccid3.c +7 -4
@@ -130,7 +130,8 @@ static u32 ccid3_hc_tx_idle_rtt(struct cstatic void ccid3_hc_tx_update_x (struct sock *sk,ktime_t *stamp) {struct ccid3_hc_tx_sock *hctx =ccid3_hc_tx_sk (sk); -u64 min_rate =2 * hctx->x_recv; +u64 min_rate =2 * hctx->x_recv, + low_thresh = ((u64 )hctx->s) <<6 ;const u64 old_x = hctx->x;ktime_t now = stamp ? *stamp :ktime_get_real ();
@@ -148,14 +149,16 @@ static void ccid3_hc_tx_update_x(structif (hctx->p >0 ) { hctx->x =min (((u64 )hctx->x_calc) <<6 , min_rate); - hctx->x =max (hctx->x, (((u64 )hctx->s) <<6 ) /TFRC_T_MBI ); + low_thresh /=TFRC_T_MBI ; }else if (ktime_us_delta (now, hctx->t_ld) - (s64 )hctx->rtt >=0 ) { - hctx->x =max (min (2 * hctx->x, min_rate), -scaled_div (((u64 )hctx->s) <<6 , hctx->rtt)); + hctx->x =min (2 * old_x, min_rate); + low_thresh =scaled_div (low_thresh, hctx->rtt); hctx->t_ld = now; } +if (hctx->x < low_thresh) + hctx->x = low_thresh;if (hctx->x != old_x) { ccid3_pr_debug("X_prev=%u, X_now=%u, X_calc=%u, "
net/dccp/ccids/lib/tfrc.h +9 -2
@@ -58,7 +58,14 @@ static inline u32 tfrc_ewma(const u32 av return avg ? (weight * avg + (10 - weight) * newval) / 10 : newval; } -extern u32 tfrc_calc_x (u16 s,u32 R ,u32 p); -extern u32 tfrc_calc_x_reverse_lookup (u32 fvalue); +extern u32 tfrc_calc_x (u16 s,u32 R ,u32 p); +extern u32 tfrc_calc_x_reverse_lookup (u32 fvalue); +extern int tfrc_tx_packet_history_init (void ); +extern void tfrc_tx_packet_history_exit (void ); +extern int tfrc_rx_packet_history_init (void ); +extern void tfrc_rx_packet_history_exit (void ); + +extern int tfrc_li_init (void ); +extern void tfrc_li_exit (void );#endif /* _TFRC_H_ */
net/dccp/ccids/lib/tfrc.c +0 -8
@@ -14,14 +14,6 @@ module_param(tfrc_debug, bool, 0444);MODULE_PARM_DESC (tfrc_debug,"Enable debug messages" );#endif -extern int tfrc_tx_packet_history_init (void ); -extern void tfrc_tx_packet_history_exit (void ); -extern int tfrc_rx_packet_history_init (void ); -extern void tfrc_rx_packet_history_exit (void ); - -extern int tfrc_li_init (void ); -extern void tfrc_li_exit (void ); -static int __init tfrc_module_init (void ) {int rc =tfrc_li_init ();
PATCH 5/7 : Consolidate duplicated code for RFC3390 conversion
While fixing a sparse warning in DCCP I found that the same code (conversion of RFC3390 bytes-to-packets) appears identically in 3 different places:
* TCP uses RFC 3390 in a packet-oriented manner (tcp_input.c);
* CCID-2 uses RFC 3390 in packet-oriented manner (RFC 4341);
* CCID-3 uses RFC 3390 in a packet-oriented manner (RFC 4342, rfc3448bis).
This patch consolidates the code common to both TCP and DCCP.
--- include/net/tcp.h | 14 ++++++++++++++ net/dccp/ccids/ccid2.c | 8 ++------ net/dccp/ccids/ccid3.c | 5 +---- net/ipv4/tcp_input.c | 17 ++--------------- 4 files changed, 19 insertions(+), 25 deletions(-)
net/ipv4/tcp_input.c +2 -15
@@ -809,25 +809,12 @@ void tcp_update_metrics(struct sock *sk) } } -/* Numbers are taken from RFC3390. -* -* John Heffner states: -* -* The RFC specifies a window of no more than 4380 bytes -* unless 2*MSS > 4380. Reading the pseudocode in the RFC -* is a bit misleading because they use a clamp at 4380 bytes -* rather than use a multiplier in the relevant range. -*/ __u32 tcp_init_cwnd (struct tcp_sock *tp,struct dst_entry *dst) {__u32 cwnd = (dst ?dst_metric (dst,RTAX_INITCWND ) :0 ); -if (!cwnd) { -if (tp->mss_cache >1460 ) - cwnd =2 ; -else - cwnd = (tp->mss_cache >1095 ) ?3 :4 ; - } +if (!cwnd) + cwnd =rfc3390_bytes_to_packets (tp->mss_cache);return min_t (__u32, cwnd, tp->snd_cwnd_clamp); }
include/net/tcp.h +15 -0
@@ -770,6 +770,21 @@ static inline __u32 tcp_current_ssthresh/* Use define here intentionally to get WARN_ON location shown at the caller */ #define tcp_verify_left_out (tp) WARN_ON(tcp_left_out(tp) > tp->packets_out) +/* +* Convert RFC3390 larger initial windows into an equivalent number of packets. +* +* John Heffner states: +* +* The RFC specifies a window of no more than 4380 bytes +* unless 2*MSS > 4380. Reading the pseudocode in the RFC +* is a bit misleading because they use a clamp at 4380 bytes +* rather than a multiplier in the relevant range. +*/ +static inline u32 rfc3390_bytes_to_packets (const u32 bytes) +{ +return bytes <1095 ?4 : (bytes >1460 ?2 :3 ); +} +extern void tcp_enter_cwr (struct sock *sk,const int set_ssthresh);extern __u32 tcp_init_cwnd (struct tcp_sock *tp,struct dst_entry *dst);
net/dccp/ccids/ccid2.c +2 -6
@@ -598,12 +598,8 @@ static int ccid2_hc_tx_init(struct ccid/* RFC 4341, 5: initialise ssthresh to arbitrarily high (max) value */ hctx->ssthresh = ~0U ; -/* -* RFC 4341, 5: "The cwnd parameter is initialized to at most four -* packets for new connections, following the rules from [RFC3390]". -* We need to convert the bytes of RFC3390 into the packets of RFC 4341. -*/ - hctx->cwnd =min (4U ,max (2U ,4380U / dp->dccps_mss_cache)); +/* Use larger initial windows (RFC 3390, rfc2581bis) */ + hctx->cwnd =rfc3390_bytes_to_packets (dp->dccps_mss_cache);/* Make sure that Ack Ratio is enabled and within bounds. */ max_ratio =DIV_ROUND_UP (hctx->cwnd,2 );
net/dccp/ccids/ccid3.c +1 -4
@@ -81,15 +81,12 @@ static void ccid3_hc_tx_set_state(struct* * X_init = min(4 * s, max(2 * s, 4380 bytes)) / RTT * -* Note that RFC 3390 uses MSS, RFC 4342 refers to RFC 3390, and rfc3448bis -* (rev-02) clarifies the use of RFC 3390 with regard to the above formula. * For consistency with other parts of the code, X_init is scaled by 2^6. */ static inline u64 rfc3390_initial_rate (struct sock *sk) {const struct ccid3_hc_tx_sock *hctx =ccid3_hc_tx_sk (sk); -const u32 w_init =min_t (u32,4 * hctx->s, -max_t (u32,2 * hctx->s,4380U )); +const u32 w_init = hctx->s *rfc3390_bytes_to_packets (hctx->s);return scaled_div (w_init <<6 , hctx->rtt); }
PATCH 6/7 : Clean up slow-path input processing
This patch rearranges the order of statements of the slow-path input processing (i.e. any other state than OPEN), to resolve the following issues.
1. Dependencies: the order of statements now better matches RFC 4340, 8.5, i.e. step 7 is before step 9 (previously 9 was before 7), and parsing options in step 8 (which can consume resources) now comes after step 7.
2. Bug-fix: in state CLOSED, there should not be any sequence number checking or option processing. This is why the test for CLOSED has been moved after the test for LISTEN.
3. As before sequence number checks are omitted if in state LISTEN/REQUEST, due to the note underneath the table in RFC 4340, 7.5.3.
4. Packets are now passed on to Ack Vector / CCID processing only after
- step 7 (receive unexpected packets),
- step 9 (receive Reset),
- step 13 (receive CloseReq),
- step 14 (receive Close)
and only if the state is PARTOPEN. This simplifies CCID processing:
- in LISTEN/CLOSED the CCIDs are non-existent;
- in RESPOND/REQUEST the CCIDs have not yet been negotiated;
- in CLOSEREQ and active-CLOSING the node has already closed this socket;
- in passive-CLOSING the client is waiting for its Reset.
In the last case, RFC 4340, 8.3 leaves it open to ignore further incoming data, which is the approach taken here.
As a result of (3), CCID processing is now indeed confined to OPEN/PARTOPEN states, i.e. congestion control is performed only on the flow of data packets.
This avoids pathological cases of doing congestion control on those messages which set up and terminate the connection.
I have done a few checks to see if this creates a problem in other parts of the code. This seems not to be the case; even if there were one, it would be better to fix it than to perform congestion control on Close/Request/Response messages. Similarly for Ack Vectors (as they depend on the negotiated CCID).
--- net/dccp/input.c | 69 ++++++++++++++++++++++++++----------------------------- 1 file changed, 33 insertions(+), 36 deletions(-)
net/dccp/input.c +33 -35
@@ -604,22 +604,36 @@ int dccp_rcv_state_process(struct sock */* Caller (dccp_v4_do_rcv) will send Reset */ dcb->dccpd_reset_code =DCCP_RESET_CODE_NO_CONNECTION ;return 1 ; + }else if (sk->sk_state =DCCP_CLOSED ) { + dcb->dccpd_reset_code =DCCP_RESET_CODE_NO_CONNECTION ; +return 1 ; } -if (sk->sk_state !=DCCP_REQUESTING && sk->sk_state !=DCCP_RESPOND ) { -if (dccp_check_seqno (sk, skb)) - goto discard; - -/* -* Step 8: Process options and mark acknowledgeable -*/ -if (dccp_parse_options (sk,NULL , skb)) - goto discard; +/* Step 6: Check sequence numbers (omitted in LISTEN/REQUEST state) */ +if (sk->sk_state !=DCCP_REQUESTING &&dccp_check_seqno (sk, skb)) + goto discard; -dccp_handle_ackvec_processing (sk, skb); -dccp_deliver_input_to_ccids (sk, skb); +/* +* Step 7: Check for unexpected packet types +* If (S.is_server and P.type = Response) +* or (S.is_client and P.type = Request) +* or (S.state = RESPOND and P.type = Data), +* Send Sync packet acknowledging P.seqno +* Drop packet and return +*/ +if ((dp->dccps_role !=DCCP_ROLE_CLIENT && + dh->dccph_type =DCCP_PKT_RESPONSE ) || + (dp->dccps_role =DCCP_ROLE_CLIENT && + dh->dccph_type =DCCP_PKT_REQUEST ) || + (sk->sk_state =DCCP_RESPOND && dh->dccph_type =DCCP_PKT_DATA )) { +dccp_send_sync (sk, dcb->dccpd_seq,DCCP_PKT_SYNC ); + goto discard; } +/* Step 8: Process options */ +if (dccp_parse_options (sk,NULL , skb)) + goto discard; +/* * Step 9: Process Reset * If P.type = Reset,
@@ -627,41 +641,21 @@ int dccp_rcv_state_process(struct sock ** S.state := TIMEWAIT * Set TIMEWAIT timer * Drop packet and return -*/ +*/ if (dh->dccph_type =DCCP_PKT_RESET ) {dccp_rcv_reset (sk, skb);return 0 ; -/* -* Step 7: Check for unexpected packet types -* If (S.is_server and P.type = Response) -* or (S.is_client and P.type = Request) -* or (S.state = RESPOND and P.type = Data), -* Send Sync packet acknowledging P.seqno -* Drop packet and return -*/ - }else if ((dp->dccps_role !=DCCP_ROLE_CLIENT && - dh->dccph_type =DCCP_PKT_RESPONSE ) || - (dp->dccps_role =DCCP_ROLE_CLIENT && - dh->dccph_type =DCCP_PKT_REQUEST ) || - (sk->sk_state =DCCP_RESPOND && - dh->dccph_type =DCCP_PKT_DATA )) { -dccp_send_sync (sk, dcb->dccpd_seq,DCCP_PKT_SYNC ); - goto discard; - }else if (dh->dccph_type =DCCP_PKT_CLOSEREQ ) { + }else if (dh->dccph_type =DCCP_PKT_CLOSEREQ ) {/* Step 13 */ if (dccp_rcv_closereq (sk, skb))return 0 ; goto discard; - }else if (dh->dccph_type =DCCP_PKT_CLOSE ) { + }else if (dh->dccph_type =DCCP_PKT_CLOSE ) {/* Step 14 */ if (dccp_rcv_close (sk, skb))return 0 ; goto discard; }switch (sk->sk_state) { -case DCCP_CLOSED : - dcb->dccpd_reset_code =DCCP_RESET_CODE_NO_CONNECTION ; -return 1 ; -case DCCP_REQUESTING : queued =dccp_rcv_request_sent_state_process (sk, skb, dh, len);if (queued >=0 )
@@ -670,8 +664,12 @@ int dccp_rcv_state_process(struct sock *__kfree_skb (skb);return 0 ; -case DCCP_RESPOND :case DCCP_PARTOPEN : +/* Step 8: if using Ack Vectors, mark packet acknowledgeable */ +dccp_handle_ackvec_processing (sk, skb); +dccp_deliver_input_to_ccids (sk, skb); +/* fall through */ +case DCCP_RESPOND : queued =dccp_rcv_respond_partopen_state_process (sk, skb, dh, len);break ;
PATCH 7/7 : Remove dead states
This patch is thanks to an investigation by Leandro Sales de Melo and his colleagues. They worked out two state diagrams which highlight the fact that the xxx_TERM states in CCID-3/4 are in fact not necessary.
And this can be confirmed by in turn looking at the code: the xxx_TERM states are only ever set in ccid3_hc_{rx,tx}_exit(). These two functions are part of the following call chain:
* ccid_hc_{tx,rx}_exit() are called from ccid_delete() only;
* ccid_delete() invokes ccid_hc_{tx,rx}_exit() in the way of a destructor:
after calling ccid_hc_{tx,rx}_exit(), the CCID is released from memory;
* ccid_delete() is in turn called only by ccid_hc_{tx,rx}_delete();
* ccid_hc_{tx,rx}_delete() is called only if
- feature negotiation failed (dccp_feat_activate_values()),
- when changing the RX/TX CCID (to eject the current CCID),
- when destroying the socket (in dccp_destroy_sock()).
In other words, when CCID-3 sets the state to xxx_TERM, it is at a time where no more processing should be going on, hence it is not necessary to introduce a dedicated exit state - this is implicit when unloading the CCID.
--- net/dccp/ccids/ccid3.c | 21 +-------------------- net/dccp/ccids/ccid3.h | 2 -- 2 files changed, 1 insertion(+), 22 deletions(-)
net/dccp/ccids/ccid3.c +1 -20
@@ -56,7 +56,6 @@ static const char *ccid3_tx_state_name(e [TFRC_SSTATE_NO_SENT] = "NO_SENT", [TFRC_SSTATE_NO_FBACK] = "NO_FBACK", [TFRC_SSTATE_FBACK] = "FBACK", - [TFRC_SSTATE_TERM] = "TERM", };return ccid3_state_names[state];
@@ -340,10 +339,6 @@ static int ccid3_hc_tx_send_packet(strucreturn (u32 )delay /1000L ;ccid3_hc_tx_update_win_count (hctx, now); -break ; -case TFRC_SSTATE_TERM : -DCCP_BUG ("%s(%p) - Illegal state TERM" ,dccp_role (sk), sk); -return -EINVAL ; }/* prepare to send now (add options etc.) */
@@ -379,10 +374,6 @@ static void ccid3_hc_tx_packet_recv(stru !(dcb->dccpd_type =DCCP_PKT_ACK || dcb->dccpd_type = DCCP_PKT_DATAACK)) return; -/* ... and only in the established state */ -if (hctx->state !=TFRC_SSTATE_FBACK && - hctx->state !=TFRC_SSTATE_NO_FBACK ) -return ; opt_recv = &hctx->options_received; now =ktime_get_real ();
@@ -557,9 +548,7 @@ static void ccid3_hc_tx_exit(struct sock {struct ccid3_hc_tx_sock *hctx =ccid3_hc_tx_sk (sk); -ccid3_hc_tx_set_state (sk,TFRC_SSTATE_TERM );sk_stop_timer (sk, &hctx->no_feedback_timer); -tfrc_tx_hist_purge (&hctx->hist); }
@@ -618,7 +607,6 @@ static const char *ccid3_rx_state_name(estatic char *ccid3_rx_state_names[] = { [TFRC_RSTATE_NO_DATA ] ="NO_DATA" , [TFRC_RSTATE_DATA ] ="DATA" , - [TFRC_RSTATE_TERM ] ="TERM" , };return ccid3_rx_state_names[state];
@@ -644,14 +632,9 @@ static void ccid3_hc_rx_send_feedback(st {struct ccid3_hc_rx_sock *hcrx =ccid3_hc_rx_sk (sk);struct dccp_sock *dp =dccp_sk (sk); -ktime_t now; +ktime_t now =ktime_get_real ();s64 delta =0 ; -if (unlikely (hcrx->state =TFRC_RSTATE_TERM )) -return ; - - now =ktime_get_real (); -switch (fbtype) {case CCID3_FBACK_INITIAL : hcrx->x_recv =0 ;
@@ -855,8 +838,6 @@ static void ccid3_hc_rx_exit(struct sock {struct ccid3_hc_rx_sock *hcrx =ccid3_hc_rx_sk (sk); -ccid3_hc_rx_set_state (sk,TFRC_RSTATE_TERM ); -tfrc_rx_hist_purge (&hcrx->hist);tfrc_lh_cleanup (&hcrx->li_hist); }
net/dccp/ccids/ccid3.h +0 -2
@@ -72,7 +72,6 @@ enum ccid3_hc_tx_states {TFRC_SSTATE_NO_SENT =1 ,TFRC_SSTATE_NO_FBACK , TFRC_SSTATE_FBACK, - TFRC_SSTATE_TERM, };/** struct ccid3_hc_tx_sock - CCID3 sender half-connection socket
@@ -127,7 +126,6 @@ static inline struct ccid3_hc_tx_sock *cenum ccid3_hc_rx_states {TFRC_RSTATE_NO_DATA =1 ,TFRC_RSTATE_DATA , -TFRC_RSTATE_TERM =127 , };/** struct ccid3_hc_rx_sock - CCID3 receiver half-connection socket
Hello Gerrit
Quoted 7 lines · Gerrit Renker
This set contains bug-fixes and clean-ups. The first two are already
in the test tree:Patch #1: Fixes a bug where CCID-3 tries to sample the RTT while the
history is empty.
Leandro - I have added the same bug fix in the CCID-4 tree, just after patch #24.
That's ok!
Quoted 4 lines
Patch #2: Deprecates the Ack Ratio sysctl as per earlier discussion.
Quoted 8 lines
The following are kept back for the moment, since the first patch is an RFC patch, and since a lot of the CCID-4 code depends on this:
Patch #3: Continues for consistency the earlier patch which removed the
long prefixes for CCID-3.
Arnaldo - can you please check. Also Leandro, can you also check this - it has implications for CCID-4, too; which is why I have not put it into the test tree immediately.
It's fine for me.
Quoted 8 lines
Patch #4: Fixes two sparse warnings.
Patch #5: Found out that RFC3390 code is identically applied in three
places (TCP, CCID-2, and CCID-3): this consolidates the duplicated code.
Note: there is a fourth place where this code is duplicated
(CCID-4), but this needs to be patched separately.
Please, see below.
Quoted 11 lines
Patch #6: Tidies up the slow-path input processing for DCCP (i.e. input
for states other than OPEN). This addresses some reordering quirks by putting steps in their RFC4340-like order, ensures that CCID/Ack Vector input is indeed confined to the fast-path, and contains one bug fix.Patch #7: Removes dead states, thanks to an observation by Leandro and
his colleagues.
Note: This would also need to be ported to CCID-4, since it
uses the same (dead) states.
You're welcome!
Quoted 4 lines
If no one shouts, I will put that into the tree approximately by the end of the week. In any case it would be good to know if the CCID-4 tree is still active, since porting these changes will take quite a bit of work.
Yes, CCID-4 is still active. I'm very busy finishing many datails for my dissertation, this includes writing the document, finishing some DCCP for GStreamer fixes, and so on. I plan to back to CCID-4 implementation in the end of April, beginning of May.
Quoted 3 lines
In particular - patch #5 may be ported to CCID-4 as well, but it depends on whether people find the code consolidation acceptable.
In my opinion it is ok.
Leandro.
Quoted 10 lines
The first two patches can be obtained in the sets from
(1) DCCP git pull dccp://eden-feed.erg.abdn.ac.uk/dccp_exp +dccp:my_dccp
(2) CCID-4 git pull dccp://eden-feed.erg.abdn.ac.uk/dccp_exp +ccid4:my_ccid4
--
To unsubscribe from this list: send the line "unsubscribe dccp" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
| > If no one shouts, I will put that into the tree approximately by the end of | > the week. In any case it would be good to know if the CCID-4 tree is still | > active, since porting these changes will take quite a bit of work. | | Yes, CCID-4 is still active. I'm very busy finishing many datails for | my dissertation, this includes writing the document, finishing some | DCCP for GStreamer fixes, and so on. I plan to back to CCID-4 | implementation in the end of April, beginning of May. |
Updated git://eden-feed.erg.abdn.ac.uk/dccp_exp [ccid4] subtree to track the recent changes in CCID-3. There are two patches in there, one mirrors the prefix change, the other aggregates all recent bug fixes and changes made to CCID-3.
This is a bit work-intensive since there are many small patches, while the actual CCID-4 code is a smaller part of that. It would be good to fold the many patches into fewer ones soon. Most of the patches also don't compile standalone (only as set), I have done some work on re-ordering chunks to fix this, but also have other work to do.
The University of Aberdeen is a charity registered in Scotland, No SC013683.