[PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[Gerrit Renker]

[CCID 3]: Simplify control flow in the calculation of t_ipi


This patch performs a simplifying (performance) optimisation:

 In each call of the inline function ccid3_calc_new_t_ipi(), the state is
 tested against TFRC_SSTATE_NO_FBACK. This is expensive when the function
 is called very often. A simpler solution, implemented by this patch, is
 to adapt the control flow.

Background:
-----------
 Upon sender initialisation, the values of t_ipi, t_nom = t_0, delta, as
 well as the initial packet sending rate, are all constants [RFC 3448, 4.2].
 Until feedback arrives, these values are not changed. Hence it is not necessary
 to recalculate t_ipi, t_nom, delta until the first feedback has arrived, i.e.
 as long as the state TFRC_SSTATE_NO_FBACK persists.

Justification:
--------------
 ccid3_calc_new_t_ipi() is called only in two places:
 
   * in ccid3_hc_tx_packet_recv(); here the state is never TFRC_SSTATE_NO_FBACK, due to
	if (hctx->ccid3hctx_state = TFRC_SSTATE_NO_FBACK) {
		ccid3_hc_tx_set_state(sk, TFRC_SSTATE_FBACK);
		/* ... */
   * in ccid3_hc_tx_packet_sent()
	  
 => Only in the second case, the state might be TFRC_SSTATE_NO_FBACK and this only 
     during the initial phase of a connection.

Solution:
---------
 This patch avoids such a recalculation by a simple change of control flow in 
 ccid3_hc_tx_packet_sent(). As a consequence, ccid3_calc_new_t_ipi() is never called in 
 the state TFRC_SSTATE_NO_FBACK, hence the test against this state can safely be removed. 

 In addition, a problematic comment in ccid3_calc_new_t_ipi() was removed:
   * the first part of the comment (initial t_ipi = 1 second) is correct
   * the second part of the comment is not correct wrt. [RFC 3448, 4.4]

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ccids/ccid3.c |    2 ++
 1 file changed, 2 insertions(+)

--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -103,13 +103,7 @@ static void ccid3_hc_tx_set_state(struct
 /* Calculate new t_ipi (inter packet interval) by t_ipi = s / X_inst */
 static inline void ccid3_calc_new_t_ipi(struct ccid3_hc_tx_sock *hctx)
 {
-	/*
-	 * If no feedback spec says t_ipi is 1 second (set elsewhere and then
-	 * doubles after every no feedback timer (separate function)
-	 */
-	if (hctx->ccid3hctx_state != TFRC_SSTATE_NO_FBACK)
-		hctx->ccid3hctx_t_ipi = usecs_div(hctx->ccid3hctx_s,
-						  hctx->ccid3hctx_x);
+	hctx->ccid3hctx_t_ipi = usecs_div(hctx->ccid3hctx_s, hctx->ccid3hctx_x);
 }
 
 /* Calculate new delta by delta = min(t_ipi / 2, t_gran / 2) */
@@ -394,6 +388,8 @@ static void ccid3_hc_tx_packet_sent(stru
 				  "as a data packet",  dccp_role(sk));
 		return;
 	case TFRC_SSTATE_NO_FBACK:
+		/* t_nom, t_ipi, delta do not change until feedback arrives */
+		return;
 	case TFRC_SSTATE_FBACK:
 		if (len > 0) {
 			timeval_sub_usecs(&hctx->ccid3hctx_t_nom,

Re: [PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[Arnaldo Carvalho de Melo]

On 11/21/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [CCID 3]: Simplify control flow in the calculation of t_ipi
>
>
> This patch performs a simplifying (performance) optimisation:
>
>  In each call of the inline function ccid3_calc_new_t_ipi(), the state is
>  tested against TFRC_SSTATE_NO_FBACK. This is expensive when the function
>  is called very often. A simpler solution, implemented by this patch, is
>  to adapt the control flow.
>
> Background:
> -----------
>  Upon sender initialisation, the values of t_ipi, t_nom = t_0, delta, as
>  well as the initial packet sending rate, are all constants [RFC 3448, 4.2].
>  Until feedback arrives, these values are not changed. Hence it is not necessary
>  to recalculate t_ipi, t_nom, delta until the first feedback has arrived, i.e.
>  as long as the state TFRC_SSTATE_NO_FBACK persists.
>
> Justification:
> --------------
>  ccid3_calc_new_t_ipi() is called only in two places:
>
>    * in ccid3_hc_tx_packet_recv(); here the state is never TFRC_SSTATE_NO_FBACK, due to
>         if (hctx->ccid3hctx_state = TFRC_SSTATE_NO_FBACK) {
>                 ccid3_hc_tx_set_state(sk, TFRC_SSTATE_FBACK);
>                 /* ... */
>    * in ccid3_hc_tx_packet_sent()
>
>  => Only in the second case, the state might be TFRC_SSTATE_NO_FBACK and this only
>      during the initial phase of a connection.
>
> Solution:
> ---------
>  This patch avoids such a recalculation by a simple change of control flow in
>  ccid3_hc_tx_packet_sent(). As a consequence, ccid3_calc_new_t_ipi() is never called in
>  the state TFRC_SSTATE_NO_FBACK, hence the test against this state can safely be removed.
>
>  In addition, a problematic comment in ccid3_calc_new_t_ipi() was removed:
>    * the first part of the comment (initial t_ipi = 1 second) is correct
>    * the second part of the comment is not correct wrt. [RFC 3448, 4.4]

Why is the second part of the comment wrong?

RFC 3448, 4.4:

4.4.  Expiration of nofeedback timer

   If the nofeedback timer expires, the sender should perform the
   following actions:

   1) Cut the sending rate in half.  If the sender has received feedback
      from the receiver, this is done by modifying the sender's cached
      copy of X_recv (the receive rate).  Because the sending rate is
      limited to at most twice X_recv, modifying X_recv limits the
      current sending rate, but allows the sender to slow-start,
      doubling its sending rate each RTT, if feedback messages resume
      reporting no losses.

If the sending rate is halved, doesn't it implies the inter packet
interval is doubled?

Otherwise I'm fine with the patch, just waiting for comments above
this specific part (comment removal).

- Arnaldo

Re: [PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[Arnaldo Carvalho de Melo]

On 11/26/06, Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:
> On 11/21/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> > [CCID 3]: Simplify control flow in the calculation of t_ipi
> >
> >
> > This patch performs a simplifying (performance) optimisation:
> >
> >  In each call of the inline function ccid3_calc_new_t_ipi(), the state is
> >  tested against TFRC_SSTATE_NO_FBACK. This is expensive when the function
> >  is called very often. A simpler solution, implemented by this patch, is
> >  to adapt the control flow.
> >
> > Background:
> > -----------
> >  Upon sender initialisation, the values of t_ipi, t_nom = t_0, delta, as
> >  well as the initial packet sending rate, are all constants [RFC 3448, 4.2].
> >  Until feedback arrives, these values are not changed. Hence it is not necessary
> >  to recalculate t_ipi, t_nom, delta until the first feedback has arrived, i.e.
> >  as long as the state TFRC_SSTATE_NO_FBACK persists.
> >
> > Justification:
> > --------------
> >  ccid3_calc_new_t_ipi() is called only in two places:
> >
> >    * in ccid3_hc_tx_packet_recv(); here the state is never TFRC_SSTATE_NO_FBACK, due to
> >         if (hctx->ccid3hctx_state = TFRC_SSTATE_NO_FBACK) {
> >                 ccid3_hc_tx_set_state(sk, TFRC_SSTATE_FBACK);
> >                 /* ... */
> >    * in ccid3_hc_tx_packet_sent()
> >
> >  => Only in the second case, the state might be TFRC_SSTATE_NO_FBACK and this only
> >      during the initial phase of a connection.
> >
> > Solution:
> > ---------
> >  This patch avoids such a recalculation by a simple change of control flow in
> >  ccid3_hc_tx_packet_sent(). As a consequence, ccid3_calc_new_t_ipi() is never called in
> >  the state TFRC_SSTATE_NO_FBACK, hence the test against this state can safely be removed.
> >
> >  In addition, a problematic comment in ccid3_calc_new_t_ipi() was removed:
> >    * the first part of the comment (initial t_ipi = 1 second) is correct
> >    * the second part of the comment is not correct wrt. [RFC 3448, 4.4]
>
> Why is the second part of the comment wrong?
>
> RFC 3448, 4.4:
>
> 4.4.  Expiration of nofeedback timer
>
>    If the nofeedback timer expires, the sender should perform the
>    following actions:
>
>    1) Cut the sending rate in half.  If the sender has received feedback
>       from the receiver, this is done by modifying the sender's cached
>       copy of X_recv (the receive rate).  Because the sending rate is
>       limited to at most twice X_recv, modifying X_recv limits the
>       current sending rate, but allows the sender to slow-start,
>       doubling its sending rate each RTT, if feedback messages resume
>       reporting no losses.
>
> If the sending rate is halved, doesn't it implies the inter packet
> interval is doubled?
>
> Otherwise I'm fine with the patch, just waiting for comments above
> this specific part (comment removal).

For now I'm merging the patch changing just the commit log not to state that the
comment was wrong, i.e. I think its just superfluous :-)

As I'm putting these csets in the 'ccid3' branch of my net-2.6.20 git
tree if you disagree about any of the things I change today (Sunday)
we can always go back and fix it before sending the whole lot to Dave.

- Arnaldo

Re: [PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[Gerrit Renker]

|  > [CCID 3]: Simplify control flow in the calculation of t_ipi
<snip>
|  >  In addition, a problematic comment in ccid3_calc_new_t_ipi() was removed:
|  >    * the first part of the comment (initial t_ipi = 1 second) is correct
|  >    * the second part of the comment is not correct wrt. [RFC 3448, 4.4]
|  
|  Why is the second part of the comment wrong?
|  
|  RFC 3448, 4.4:
|  
|  4.4.  Expiration of nofeedback timer
|  
|     If the nofeedback timer expires, the sender should perform the
|     following actions:
|  
|     1) Cut the sending rate in half.  If the sender has received feedback
|        from the receiver, this is done by modifying the sender's cached
|        copy of X_recv (the receive rate).  Because the sending rate is
|        limited to at most twice X_recv, modifying X_recv limits the
|        current sending rate, but allows the sender to slow-start,
|        doubling its sending rate each RTT, if feedback messages resume
|        reporting no losses.
|  
|  If the sending rate is halved, doesn't it implies the inter packet
|  interval is doubled?
Yes that is right. Ian, I apologise - you were correct in this comment. 
Will re-send an update.

Re: [PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[Gerrit Renker]

Quoting Arnaldo Carvalho de Melo:
|  For now I'm merging the patch changing just the commit log not to state that the
|  comment was wrong, i.e. I think its just superfluous :-)
|  
|  As I'm putting these csets in the 'ccid3' branch of my net-2.6.20 git
|  tree if you disagree about any of the things I change today (Sunday)
|  we can always go back and fix it before sending the whole lot to Dave.
If you haven't committed these (they are not in your tree yet), can you please
consider the attached update where I have corrected commit message?
Will put this online, too.

--------------------> Updated Patch <--------------------------------------
[CCID 3]: Simplify control flow in the calculation of t_ipi


This patch performs a simplifying (performance) optimisation:

 In each call of the inline function ccid3_calc_new_t_ipi(), the state is
 tested against TFRC_SSTATE_NO_FBACK. This is expensive when the function
 is called very often. A simpler solution, implemented by this patch, is
 to adapt the control flow.

Background:
-----------
 Upon sender initialisation, the values of t_ipi, t_nom = t_0, delta, as
 well as the initial packet sending rate, are all constants [RFC 3448, 4.2].
 Until feedback arrives, these values are not changed. Hence it is not necessary
 to recalculate t_ipi, t_nom, delta until the first feedback has arrived, i.e.
 as long as the state TFRC_SSTATE_NO_FBACK persists.

Justification:
--------------
 ccid3_calc_new_t_ipi() is called only in two places:
 
   * in ccid3_hc_tx_packet_recv(); here the state is never TFRC_SSTATE_NO_FBACK, due to
	if (hctx->ccid3hctx_state = TFRC_SSTATE_NO_FBACK) {
		ccid3_hc_tx_set_state(sk, TFRC_SSTATE_FBACK);
		/* ... */
   * in ccid3_hc_tx_packet_sent()
	  
 => Only in the second case, the state might be TFRC_SSTATE_NO_FBACK and this only 
     during the initial phase of a connection.

Solution:
---------
 This patch avoids such a recalculation by a simple change of control flow in 
 ccid3_hc_tx_packet_sent(). As a consequence, ccid3_calc_new_t_ipi() is never called in 
 the state TFRC_SSTATE_NO_FBACK, hence the test against this state can safely be removed. 
 Additionally, a comment at the begin of this function is removed.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ccids/ccid3.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -103,13 +103,7 @@ static void ccid3_hc_tx_set_state(struct
 /* Calculate new t_ipi (inter packet interval) by t_ipi = s / X_inst */
 static inline void ccid3_calc_new_t_ipi(struct ccid3_hc_tx_sock *hctx)
 {
-	/*
-	 * If no feedback spec says t_ipi is 1 second (set elsewhere and then
-	 * doubles after every no feedback timer (separate function)
-	 */
-	if (hctx->ccid3hctx_state != TFRC_SSTATE_NO_FBACK)
-		hctx->ccid3hctx_t_ipi = usecs_div(hctx->ccid3hctx_s,
-						  hctx->ccid3hctx_x);
+	hctx->ccid3hctx_t_ipi = usecs_div(hctx->ccid3hctx_s, hctx->ccid3hctx_x);
 }
 
 /* Calculate new delta by delta = min(t_ipi / 2, t_gran / 2) */
@@ -395,6 +389,8 @@ static void ccid3_hc_tx_packet_sent(stru
 				  "as a data packet",  dccp_role(sk));
 		return;
 	case TFRC_SSTATE_NO_FBACK:
+		/* t_nom, t_ipi, delta do not change until feedback arrives */
+		return;
 	case TFRC_SSTATE_FBACK:
 		if (len > 0) {
 			timeval_sub_usecs(&hctx->ccid3hctx_t_nom,

Re: [PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[Arnaldo Carvalho de Melo]

On 11/27/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> Quoting Arnaldo Carvalho de Melo:
> |  For now I'm merging the patch changing just the commit log not to state that the
> |  comment was wrong, i.e. I think its just superfluous :-)
> |
> |  As I'm putting these csets in the 'ccid3' branch of my net-2.6.20 git
> |  tree if you disagree about any of the things I change today (Sunday)
> |  we can always go back and fix it before sending the whole lot to Dave.
> If you haven't committed these (they are not in your tree yet), can you please

Its in the 'ccid3' branch, look at the bottoom of:

http://www.kernel.org/git/?p=linux/kernel/git/acme/net-2.6.20.git;a=summary

Or directly in:

http://www.kernel.org/git/?p=linux/kernel/git/acme/net-2.6.20.git;a=shortlog;hÌid3

But as I said in one of my messages yesterday, this one was just a
first stab at merging your ccid3 work, I'm now getting back to getting
the ones where there was general agreement to submit to Dave, then
look again at the others.

> consider the attached update where I have corrected commit message?

I'll use the updated one.

> Will put this online, too.

Thanks,

- Arnaldo

Re: [PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[Gerrit Renker]

> If you haven't committed these (they are not in your tree yet)

|  Its in the 'ccid3' branch, look at the bottoom of:
|  
|  http://www.kernel.org/git/?p=linux/kernel/git/acme/net-2.6.20.git;a=summary
|  
|  Or directly in:
|  
|  http://www.kernel.org/git/?p=linux/kernel/git/acme/net-2.6.20.git;a=shortlog;hÌid3
I should have used these urls ... %-)
I pulled your tree and it was not `in' and so was lead on the wrong track. Have bookmarked
them - very handy to have. 

Re: [PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[Ian McDonald]

On 11/28/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [CCID 3]: Simplify control flow in the calculation of t_ipi
>
>
Acked-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
-- 
Ian McDonald
Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group
Department of Computer Science
University of Waikato
New Zealand

Re: [PATCH 2/6]: Simplify control flow in the calculation of t_ipi

[Arnaldo Carvalho de Melo]

On 11/27/06, Ian McDonald <ian.mcdonald@jandi.co.nz> wrote:
> On 11/28/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> > [CCID 3]: Simplify control flow in the calculation of t_ipi
> >
> >
> Acked-by: Ian McDonald <ian.mcdonald@jandi.co.nz>

Thanks, but this one was the only one you hadn't acked when I
submitted latest batch to Dave, and he already pulled it, thanks
anyway!

- Arnaldo