Comments/question on no_feedback_timer

Comments/question on no_feedback_timer

[Ian McDonald]

I've got a few comments/questions on the no feedback timer on CCID3.
It's probably me not reading the code properly and I've got no idea
where the code came from (may have even been us!!)


In this code from ccid3_hc_tx_no_feedback_timer we work on x_recv
mostly rather than x. I'm wondering why??
		/*
		 *  Modify the cached value of X_recv
		 *
		 *  If (X_calc > 2 * X_recv)
		 *    X_recv = max(X_recv / 2, s / (2 * t_mbi));
		 *  Else
		 *    X_recv = X_calc / 4;
		 *
		 *  Note that X_recv is scaled by 2^6 while X_calc is not
		 */
		BUG_ON(hctx->ccid3hctx_p && !hctx->ccid3hctx_x_calc);

		if (hctx->ccid3hctx_x_calc > (hctx->ccid3hctx_x_recv >> 5))
			hctx->ccid3hctx_x_recv 				max(hctx->ccid3hctx_x_recv / 2,
				    (((__u64)hctx->ccid3hctx_s) << 6) /
							      (2 * TFRC_T_MBI));
		else {
			hctx->ccid3hctx_x_recv = hctx->ccid3hctx_x_calc;
			hctx->ccid3hctx_x_recv <<= 4;
		}
		ccid3_hc_tx_update_x(sk, NULL);

and I think this then is potentially problematic when
ccid3_hc_tx_update_x if it is less than 2 rtt idle and we go past this
code:
	if (ccid3_hc_tx_idle_rtt(hctx, now) >= 2) {
		min_rate = rfc3390_initial_rate(sk);
		min_rate = max(min_rate, 2 * hctx->ccid3hctx_x_recv);
	}

because we then go through to the rest of the code and we can
effectively double the rate (min_rate is doubled at start of
ccid3_hc_tx_update_x).

This seems bizarre to me. We get no feedback and we double the rate??
I know it will get fixed in short order when we detect the loss later
on but this just seems wrong.

Can someone else verify before I fix? I do have a habit of reading
code wrong so would like verification.

Ian
-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: Comments/question on no_feedback_timer

[Gerrit Renker]

Ian,

to clarify - all rfc3448bis 

It is consistent with revision 00 of draft rfc3448bis; check against:
 http://tools.ietf.org/html/draft-ietf-dccp-rfc3448bis-00#section-4.4



Quoting Ian McDonald:
|  I've got a few comments/questions on the no feedback timer on CCID3.
|  It's probably me not reading the code properly and I've got no idea
|  where the code came from (may have even been us!!)
|  
|  
|  In this code from ccid3_hc_tx_no_feedback_timer we work on x_recv
|  mostly rather than x. I'm wondering why??
|  		/*
|  		 *  Modify the cached value of X_recv
|  		 *
|  		 *  If (X_calc > 2 * X_recv)
|  		 *    X_recv = max(X_recv / 2, s / (2 * t_mbi));
|  		 *  Else
|  		 *    X_recv = X_calc / 4;
|  		 *
|  		 *  Note that X_recv is scaled by 2^6 while X_calc is not
|  		 */
|  		BUG_ON(hctx->ccid3hctx_p && !hctx->ccid3hctx_x_calc);
|  
|  		if (hctx->ccid3hctx_x_calc > (hctx->ccid3hctx_x_recv >> 5))
|  			hctx->ccid3hctx_x_recv |  				max(hctx->ccid3hctx_x_recv / 2,
|  				    (((__u64)hctx->ccid3hctx_s) << 6) /
|  							      (2 * TFRC_T_MBI));
|  		else {
|  			hctx->ccid3hctx_x_recv = hctx->ccid3hctx_x_calc;
|  			hctx->ccid3hctx_x_recv <<= 4;
|  		}
|  		ccid3_hc_tx_update_x(sk, NULL);
Here the relevant passage is 
 4.4 Expiration of nofeedback timer

	"If (X_calc > 2*X_recv)
                X_recv = max(X_recv/2, s/(2*t_mbi));
         Else
                X_recv = X_calc/4;"

This corresponds one-to-one with the above code, given that X_recv is scaled by 2^6.
  
|  and I think this then is potentially problematic when
|  ccid3_hc_tx_update_x if it is less than 2 rtt idle and we go past this
|  code:
|  	if (ccid3_hc_tx_idle_rtt(hctx, now) >= 2) {
|  		min_rate = rfc3390_initial_rate(sk);
|  		min_rate = max(min_rate, 2 * hctx->ccid3hctx_x_recv);
|  	}
|  
|  because we then go through to the rest of the code and we can
|  effectively double the rate (min_rate is doubled at start of
|  ccid3_hc_tx_update_x).
This corresponds to section 4.3 of above URL:

    "4)  Update the sending rate as follows:

             If (sender has been idle or data-limited)
                 min_rate = max(2*X_recv, W_init/R);
             Else
                 min_rate = 2*X_recv;
    "
Again there is a one-to-one correspondence, given the initialisation statement of

    __u64 min_rate = 2 * hctx->ccid3hctx_x_recv;
  
|  This seems bizarre to me. We get no feedback and we double the rate??
|  I know it will get fixed in short order when we detect the loss later
|  on but this just seems wrong.
Here is a misunderstanding. ccid3_hc_tx_idle_rtt() measures the amount of time
the sender has not sent anything, returned as integral number of full RTTs. 
This notion of idling is consistent with section 6.4 of RFC 4342 and further 
consistent with the definition of CCID2's quiescence in section 6.2.1 of RFC 4341.

If it still seems wrong, then this is something on the level of the specification.
It has taken painstaking effort to align the implementation with RFC 4342, 4340,
3448, rfc3448bis. I am quite reluctant to start tearing this apart, since it can
not be done `fix' wise: if you do it, then the complete set of changes from rev00
to rev02 of rfc3448bis needs to be implemented.

|  Can someone else verify before I fix? I do have a habit of reading
|  code wrong so would like verification. 
I repeat, the code is consistent with revision 00 of draft rfc3448bis, available
from http://tools.ietf.org/html/draft-ietf-dccp-rfc3448bis-00

Given the recent discussions of this, I would like to postpone such discussion until
December, after they have reviewed the next revision of rfc3448bis.

Re: Comments/question on no_feedback_timer

[Ian McDonald]

On 8/3/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> Ian,
>
> to clarify - all rfc3448bis
>
> It is consistent with revision 00 of draft rfc3448bis; check against:
>  http://tools.ietf.org/html/draft-ietf-dccp-rfc3448bis-00#section-4.4
>
OK there is my issue partially. I'm working against revision 02 and
that is quite different :-(

> Here is a misunderstanding. ccid3_hc_tx_idle_rtt() measures the amount of time
> the sender has not sent anything, returned as integral number of full RTTs.
> This notion of idling is consistent with section 6.4 of RFC 4342 and further
> consistent with the definition of CCID2's quiescence in section 6.2.1 of RFC 4341.
>
> If it still seems wrong, then this is something on the level of the specification.
> It has taken painstaking effort to align the implementation with RFC 4342, 4340,
> 3448, rfc3448bis. I am quite reluctant to start tearing this apart, since it can
> not be done `fix' wise: if you do it, then the complete set of changes from rev00
> to rev02 of rfc3448bis needs to be implemented.
>
Yes I understand.

> |  Can someone else verify before I fix? I do have a habit of reading
> |  code wrong so would like verification.
> I repeat, the code is consistent with revision 00 of draft rfc3448bis, available
> from http://tools.ietf.org/html/draft-ietf-dccp-rfc3448bis-00
>
> Given the recent discussions of this, I would like to postpone such discussion until
> December, after they have reviewed the next revision of rfc3448bis.
>
OK. I don't intend to get into long discussions around individual
versions or discuss to death. I just didn't realise which version the
code was against. It creates a headache for me but that's my
problem...

Ian
-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: Comments/question on no_feedback_timer

[Ian McDonald]

> On 8/3/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> > Given the recent discussions of this, I would like to postpone such discussion until
> > December, after they have reviewed the next revision of rfc3448bis.
> >

Ian said:
> OK. I don't intend to get into long discussions around individual
> versions or discuss to death. I just didn't realise which version the
> code was against. It creates a headache for me but that's my
> problem...
>

And the more I look at it the bigger my headache. I'm going to have to
start doing some of the TFRCbis rev02 stuff given the intertwining
with Faster Restart. I won't discuss as such given your concerns but
I'll probably post back patches from time to time that you can do what
you want with. (Ignore, integrate or somewhere in between)

Ian
-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: Comments/question on no_feedback_timer

[Gerrit Renker]

|  And the more I look at it the bigger my headache. I'm going to have to
|  start doing some of the TFRCbis rev02 stuff given the intertwining
|  with Faster Restart.
I take it that you are aware that the authors of the Faster Restart draft
are in the process of replacing the current version with an entirely different one
based on cwnd window validation. I'd check with the authors for their time plan.

Having rev02 in full length (and provided feedback), I found other issues in rev02.
There are a couple of things (such as receive-sets) which make implementation even more 
complex. Plus, the situation is like Faster-Restart: the draft is in a volatile state, 
some feedback (after recent IETF) seems to not have been addressed, the next rev is due 
in December, and only after the IETF meeting can some convergence/clarity be expected.

|  I'll probably post back patches from time to time that you can do what
|  you want with. (Ignore, integrate or somewhere in between)
I'd say, we put this up as a fork of the DCCP test tree? 

If you encounter issues or problems with the test tree and there are things 
            - outside the rev00 and rev02 discussion -
which can make the test tree better, then of course this goes into the test tree.

But due to the entanglements, I'd much prefer to keep rev00 and rev02 code separate
from each other.

Re: Comments/question on no_feedback_timer

[Ian McDonald]

On 8/7/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> |  And the more I look at it the bigger my headache. I'm going to have to
> |  start doing some of the TFRCbis rev02 stuff given the intertwining
> |  with Faster Restart.
> I take it that you are aware that the authors of the Faster Restart draft
> are in the process of replacing the current version with an entirely different one
> based on cwnd window validation. I'd check with the authors for their time plan.
>
No! I just checked Sally's slides from IETF meeting held and it didn't
mention this at all. Where did you get this from?

> Having rev02 in full length (and provided feedback), I found other issues in rev02.
> There are a couple of things (such as receive-sets) which make implementation even more
> complex. Plus, the situation is like Faster-Restart: the draft is in a volatile state,
> some feedback (after recent IETF) seems to not have been addressed, the next rev is due
> in December, and only after the IETF meeting can some convergence/clarity be expected.
>
Yes I agree receive sets are complex. I didn't see your feedback -
maybe I didn't look properly - can you send to me?

> |  I'll probably post back patches from time to time that you can do what
> |  you want with. (Ignore, integrate or somewhere in between)
> I'd say, we put this up as a fork of the DCCP test tree?
>
However you want to handle is fine.

> If you encounter issues or problems with the test tree and there are things
>             - outside the rev00 and rev02 discussion -
> which can make the test tree better, then of course this goes into the test tree.
>
> But due to the entanglements, I'd much prefer to keep rev00 and rev02 code separate
> from each other.
>
Understand.

-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: Comments/question on no_feedback_timer

[Gerrit Renker]

Quoting Ian McDonald:
|  > |  And the more I look at it the bigger my headache. I'm going to have to
|  > |  start doing some of the TFRCbis rev02 stuff given the intertwining
|  > |  with Faster Restart.
|  > I take it that you are aware that the authors of the Faster Restart draft
|  > are in the process of replacing the current version with an entirely different one
|  > based on cwnd window validation. I'd check with the authors for their time plan.
|  >
|  No! I just checked Sally's slides from IETF meeting held and it didn't
|  mention this at all. Where did you get this from?
|
This is something I heard - can you please check this yourself with the authors. Faster Restart 
is not really my domain and I don't want to give warranties for things I heard somewhere and may
have misunderstood (or maybe I got it right, but do check!).
  
|  Yes I agree receive sets are complex. I didn't see your feedback -
|  maybe I didn't look properly - can you send to me?
This had been sent internally. Will send to you privately - this is an implementation list.