[PATCH 3/3]: Update computation of X to use newer timeofday interface

[PATCH 3/3]: Update computation of X to use newer timeofday interface

[Gerrit Renker]

[CCID3]: Update computation of X to use newer timeofday interface

This updates the timestamping references in the computation of X to the newer
ktime_t interface. The time-last-doubled t_ld is now also upgraded to ktime_t;
furthermore up-to-date comments were added to ccid3_hc_tx_update_x().

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ccids/ccid3.c |   23 ++++++++++-------------
 net/dccp/ccids/ccid3.h |    2 +-
 2 files changed, 11 insertions(+), 14 deletions(-)

--- a/net/dccp/ccids/ccid3.h
+++ b/net/dccp/ccids/ccid3.h
@@ -110,7 +110,7 @@ struct ccid3_hc_tx_sock {
 	u8				ccid3hctx_idle;
 	ktime_t				ccid3hctx_t_last_win_count;
 	struct timer_list		ccid3hctx_no_feedback_timer;
-	struct timeval			ccid3hctx_t_ld;
+	ktime_t				ccid3hctx_t_ld;
 	ktime_t				ccid3hctx_t_nom;
 	u32				ccid3hctx_delta;
 	struct list_head		ccid3hctx_hist;
--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -114,27 +114,24 @@ static inline void ccid3_update_send_int
 		       hctx->ccid3hctx_s, (unsigned)(hctx->ccid3hctx_x >> 6));
 
 }
-/*
- * Update X by
- *    If (p > 0)
- *       X_calc = calcX(s, R, p);
- *       X = max(min(X_calc, 2 * X_recv), s / t_mbi);
- *    Else
- *       If (now - tld >= R)
- *          X = max(min(2 * X, 2 * X_recv), s / R);
- *          tld = now;
+
+/**
+ * ccid3_hc_tx_update_x  -  Update allowed sending rate X
+ * @stamp: most recent time if available - can be left NULL.
+ * This function tracks draft rfc3448bis, check there for latest details.
  *
  * Note: X and X_recv are both stored in units of 64 * bytes/second, to support
  *       fine-grained resolution of sending rates. This requires scaling by 2^6
  *       throughout the code. Only X_calc is unscaled (in bytes/second).
  *
  */
-static void ccid3_hc_tx_update_x(struct sock *sk, struct timeval *now)
+static 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->ccid3hctx_x_recv;
 	const  __u64 old_x = hctx->ccid3hctx_x;
+	ktime_t now = stamp? *stamp : ktime_get_real();
 
 	/*
 	 * Handle IDLE periods: do not reduce below RFC3390 initial sending rate
@@ -154,14 +151,14 @@ static void ccid3_hc_tx_update_x(struct 
 					(((__u64)hctx->ccid3hctx_s) << 6) /
 								TFRC_T_MBI);
 
-	} else if (timeval_delta(now, &hctx->ccid3hctx_t_ld) -
-			(suseconds_t)hctx->ccid3hctx_rtt >= 0) {
+	} else if (ktime_delta(now, hctx->ccid3hctx_t_ld)
+				- (s64)hctx->ccid3hctx_rtt >= 0) {
 
 		hctx->ccid3hctx_x  			max(min(2 * hctx->ccid3hctx_x, min_rate),
 			    scaled_div(((__u64)hctx->ccid3hctx_s) << 6,
 				       hctx->ccid3hctx_rtt));
-		hctx->ccid3hctx_t_ld = *now;
+		hctx->ccid3hctx_t_ld = now;
 	}
 
 	if (hctx->ccid3hctx_x != old_x) {

Re: [PATCH 3/3]: Update computation of X to use newer timeofday interface

[Ian McDonald]

On 6/10/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [CCID3]: Update computation of X to use newer timeofday interface
>
> This updates the timestamping references in the computation of X to the newer
> ktime_t interface. The time-last-doubled t_ld is now also upgraded to ktime_t;
> furthermore up-to-date comments were added to ccid3_hc_tx_update_x().
>
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>

This looks OK but...
> -static void ccid3_hc_tx_update_x(struct sock *sk, struct timeval *now)
> +static void ccid3_hc_tx_update_x(struct sock *sk, ktime_t *stamp)
>
I don't see callers updated. Is this what you mean by patches being
interdependent?

Ian
-- 
Web: http://wand.net.nz/~iam4/
Blog: http://iansblog.jandi.co.nz
WAND Network Research Group

Re: [PATCH 3/3]: Update computation of X to use newer timeofday interface

[Gerrit Renker]

Quoting Ian McDonald:
|  This looks OK but...
|  > -static void ccid3_hc_tx_update_x(struct sock *sk, struct timeval *now)
|  > +static void ccid3_hc_tx_update_x(struct sock *sk, ktime_t *stamp)
|  >
|  I don't see callers updated. Is this what you mean by patches being
|  interdependent?
That is right - the callers are updated in the next bundle of patches. This
set contains 29 patches in total, divided into the following sets:
 1. ktime_t update                   [sent]
 2. RTT and timestamping patches     [sent]
 3. CCID3 TX history - locking and migration to singly-linked list
   (suggestion by Arnaldo)                          => to be sent
 4. Update of computation wrt draft rfc3448bis-00   => to be sent
 5. Miscellaneous CCID3 patches                     => to be sent
 6. Miscellaneous DCCP patches                      => to be sent

The callers are updated in subset (4) - I will send that later today.

These patches are first and foremost meant for the upcoming `experimental' tree,
I was thinking that sending one or two bundles at a time is better than sending
the whole 29 patches at once. 

Once they are in the tree, after going through this stage, the relationships are
easier to see. 

Thus, interdependencies are unavoidable at this stage, but they have been minimised
to quite a great degree: with some labour, 5 patches that overlapped with others
were merged across 50 or so patches. 

Doing even more integration would make them less readable.

Re: [PATCH 3/3]: Update computation of X to use newer timeofday interface

[Arnaldo Carvalho de Melo]

On 6/11/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> Quoting Ian McDonald:
> |  This looks OK but...
> |  > -static void ccid3_hc_tx_update_x(struct sock *sk, struct timeval *now)
> |  > +static void ccid3_hc_tx_update_x(struct sock *sk, ktime_t *stamp)
> |  >
> |  I don't see callers updated. Is this what you mean by patches being
> |  interdependent?
> That is right - the callers are updated in the next bundle of patches. This
> set contains 29 patches in total, divided into the following sets:
>  1. ktime_t update                   [sent]
>  2. RTT and timestamping patches     [sent]
>  3. CCID3 TX history - locking and migration to singly-linked list
>    (suggestion by Arnaldo)                          => to be sent
>  4. Update of computation wrt draft rfc3448bis-00   => to be sent
>  5. Miscellaneous CCID3 patches                     => to be sent
>  6. Miscellaneous DCCP patches                      => to be sent
>
> The callers are updated in subset (4) - I will send that later today.
>
> These patches are first and foremost meant for the upcoming `experimental' tree,
> I was thinking that sending one or two bundles at a time is better than sending
> the whole 29 patches at once.
>
> Once they are in the tree, after going through this stage, the relationships are
> easier to see.
>
> Thus, interdependencies are unavoidable at this stage, but they have been minimised
> to quite a great degree: with some labour, 5 patches that overlapped with others
> were merged across 50 or so patches.
>
> Doing even more integration would make them less readable.

We'll have to avoid it, I'm looking at how to do it...


I'm just stuck with t_ld not having being converted to ktime_t, so
I'll fix this up by...

-                       hctx->ccid3hctx_t_ld = now;
+                       hctx->ccid3hctx_t_ld = ktime_to_timeval(now);

With this the second patch builds, now to look at the 3rd.


Gerrit, have you ever used git-bisect? Think about what people that
build distro kernels, where our DCCP stuff is built, will think about
us when doing a bisect to find the changeset that introduced a bug in
a completely unrelated area such as sysrq+M oopsing on machines with
sparse memory maps (example: myself last week 8) ) and the build
breaks because of such patch interdependency...

So I'll try to help you in finding ways for never, ever having the
tree not building at any point in time.

This is OK when in a rush and in a private tree, not in something that
we expect to merge :-)

- Arnaldo

Re: [PATCH 3/3]: Update computation of X to use newer timeofday interface

[Gerrit Renker]

Quoting Arnaldo Carvalho de Melo:
|  We'll have to avoid it, I'm looking at how to do it...
|  
|  
|  I'm just stuck with t_ld not having being converted to ktime_t, so
|  I'll fix this up by...
|  
|  -                       hctx->ccid3hctx_t_ld = now;
|  +                       hctx->ccid3hctx_t_ld = ktime_to_timeval(now);
|  
|  With this the second patch builds, now to look at the 3rd.
|  
|  
|  Gerrit, have you ever used git-bisect? Think about what people that
|  build distro kernels, where our DCCP stuff is built, will think about
|  us when doing a bisect to find the changeset that introduced a bug in
|  a completely unrelated area such as sysrq+M oopsing on machines with
|  sparse memory maps (example: myself last week 8) ) and the build
|  breaks because of such patch interdependency...
|  
|  So I'll try to help you in finding ways for never, ever having the
|  tree not building at any point in time.
|  
|  This is OK when in a rush and in a private tree, not in something that
|  we expect to merge :-)
In that case please don't consider these patches for merging. I may not
have been clear, but these patches are first and foremost destined for
the experimental tree, and there are still 12 patches outstanding.

I.e. you are trying to build on something which is not yet complete.

We had these inter-dependencies before and they have been giving me (and
from what I read not only me) a lot of hell. It is all inter-related
and it is extremely hard to make patches both readable, split in logical
units, and compile as well.

I agree with your point, but trying to dissect these patches while in the
middle of a submission is madness. 

My idea was to put that into the test tree and offer it for testing.

I myself have been testing this code for 4..5 months now with good results.

I appreciate your help but I think this patch set can wait - at least until
I have had time to submit the rest, please.

Thanks.

Re: [PATCH 3/3]: Update computation of X to use newer timeofday interface

[Arnaldo Carvalho de Melo]

On 6/16/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> Quoting Arnaldo Carvalho de Melo:
> |  We'll have to avoid it, I'm looking at how to do it...
> |
> |
> |  I'm just stuck with t_ld not having being converted to ktime_t, so
> |  I'll fix this up by...
> |
> |  - hctx->ccid3hctx_t_ld = now;
> |  + hctx->ccid3hctx_t_ld = ktime_to_timeval(now);
> |
> |  With this the second patch builds, now to look at the 3rd.
> |
> |
> |  Gerrit, have you ever used git-bisect? Think about what people that
> |  build distro kernels, where our DCCP stuff is built, will think about
> |  us when doing a bisect to find the changeset that introduced a bug in
> |  a completely unrelated area such as sysrq+M oopsing on machines with
> |  sparse memory maps (example: myself last week 8) ) and the build
> |  breaks because of such patch interdependency...
> |
> |  So I'll try to help you in finding ways for never, ever having the
> |  tree not building at any point in time.
> |
> |  This is OK when in a rush and in a private tree, not in something that
> |  we expect to merge :-)
> In that case please don't consider these patches for merging. I may not
> have been clear, but these patches are first and foremost destined for
> the experimental tree, and there are still 12 patches outstanding.

OK, but doing the simple ktime_to_timeval above I was able to merge
something that moves us further, its clear and self contained, so I
loved it and merged.

>
> I.e. you are trying to build on something which is not yet complete.

I made it self contained (the second patch), split it so that the one
after it fixes the bug you described, which improves what we have in
the tree.

> We had these inter-dependencies before and they have been giving me (and
> from what I read not only me) a lot of hell. It is all inter-related
> and it is extremely hard to make patches both readable, split in logical
> units, and compile as well.

I know,

> I agree with your point, but trying to dissect these patches while in the
> middle of a submission is madness.

Hey, think that what we're doing is: you're working on the
experimental tree, I'm trying to cherry pick what I think its small,
self contained and fixes things, reducing the size of the experimental
tree.

> My idea was to put that into the test tree and offer it for testing.
>
> I myself have been testing this code for 4..5 months now with good results.

Thank you for that!

> I appreciate your help but I think this patch set can wait - at least until
> I have had time to submit the rest, please.

I'll continue reading it, and will discuss with you in advance the
ones I think I can merge, ok?

- Arnaldo

Re: [PATCH 3/3]: Update computation of X to use newer timeofday interface

[Gerrit Renker]

|  OK, but doing the simple ktime_to_timeval above I was able to merge
|  something that moves us further, its clear and self contained, so I
|  loved it and merged.
Oh, sorry - I think I misunderstood you. Yes, of course, I was talking
experimental tree and didn't exactly know what you were working on.

And I fully trust that the results will be good. I will be back with
an update and as soon as the patch set is complete, I will bring it up
in the experimental tree. 

As in a recent email exchange with Ian, I believe that in the future
this will be much easier.

And if you have ideas or suggestions where patches could better be cut,
I'd be happy to take them on board.