: 3f_CCID3_refactor_loss_interval_code_McDonald.diff

I would like to retract the change in the interface of update_li which we discussed recently.

The reason is that the caller supplies `loss' parameters (a loss sequence number and a loss CCVal); the fact that in the current implementation this coincides with the fields

       hccrx->ccid3hcrx_seqno_nonloss and
       hcrx->ccid3hcrx_ccval_nonloss

in ccid3_hc_rx_detect_loss is more of a coincidence. I have been going over this code several times and come to the conclusion that not changing the interface of update_li is the cleanest way. I have uploaded this to the online directory, below are the differences to Ian's original.

==> This is the `meta-diff' between original diff and the new diff:

@@ -842,7 +842,8 @@
 	while (dccp_delta_seqno(hcrx->ccid3hcrx_seqno_nonloss, seqno)
 	   > TFRC_RECV_NUM_LATE_LOSS) {
 		loss = 1;
-		dccp_li_update_li(sk);
+		dccp_li_update_li(sk, hcrx->ccid3hcrx_seqno_nonloss,
+		                      hcrx->ccid3hcrx_ccval_nonloss);
 		tmp_seqno = hcrx->ccid3hcrx_seqno_nonloss;
 		dccp_inc_seqno(&tmp_seqno);
 		hcrx->ccid3hcrx_seqno_nonloss = tmp_seqno;

@@ -248,13 +248,11 @@
 		return 1000000 / p;
 }
 
-void dccp_li_update_li(struct sock *sk)
+void dccp_li_update_li(struct sock *sk, u64 seq_loss, u8 win_loss)
 {
 	struct ccid3_hc_rx_sock *hcrx = ccid3_hc_rx_sk(sk);
 	struct dccp_li_hist_entry *head;
 	u64 seq_temp;
-	u64 seq_loss = hcrx->ccid3hcrx_seqno_nonloss;
-	u8 win_loss = hcrx->ccid3hcrx_ccval_nonloss;
 
 	if (list_empty(&hcrx->ccid3hcrx_li_hist)) {
 		if (!dccp_li_hist_interval_new(&hcrx->ccid3hcrx_li_hist,

@@ -36,3 +36,3 @@
 
-extern void dccp_li_update_li(struct sock *sk);
+extern void dccp_li_update_li(struct sock *sk, u64 seq_loss, u8 win_loss);
 #endif /* _DCCP_LI_HIST_ */

I disagree with you on this. It's not a coincidence at all. I planned the code that way. It "happens" to have the right values because I put them there.

I disagree with this. However I can see some confusion because we are equating nonloss and loss variables and the variables are named badly in the loss interval code. What I've done is stuck with my original patch but changed the variable names seq_loss and win_loss to seq_nonloss and win_nonloss respectively.

I've posted the new version online. Can you now use this one please?

Ian

Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group

Quoting Ian McDonald:

|  On 1/6/07, Gerrit Renker <[email protected]> wrote:
|  > I would like to retract the change in the interface of update_li which we discussed recently.
|  >
|  > The reason is that the caller supplies `loss' parameters (a loss sequence number and a loss
|  > CCVal); the fact that in the current implementation this coincides with the fields
|  >
|  >        hccrx->ccid3hcrx_seqno_nonloss and
|  >        hcrx->ccid3hcrx_ccval_nonloss
|  >
|  > in ccid3_hc_rx_detect_loss is more of a coincidence.
|  
|  I disagree with you on this. It's not a coincidence at all. I planned
|  the code that way. It "happens" to have the right values because I put
|  them there.
|  
|  > I have been going over this code several
|  > times and come to the conclusion that not changing the interface of update_li is the cleanest
|  > way. I have uploaded this to the online directory, below are the differences to Ian's original.
|  >
|  >
|  I disagree with this. However I can see some confusion because we are
|  equating nonloss and loss variables and the variables are named badly
|  in the loss interval code. What I've done is stuck with my original
|  patch but changed the variable names seq_loss and win_loss to
|  seq_nonloss and win_nonloss respectively.
|  
|  I've posted the new version online. Can you now use this one please?

You are right - the way it is used corresponds to the highest sequence number received before the loss. Therefore of course it is nonloss what we are storing. Thanks for the explanation, I have downloaded your patch, refreshed it with regard to offset etc, acked, and uploaded again as 3f.

Thanks
Gerrit