Skip to contentSkip to conversation

: Update computation of X to use newer timeofday interface

PATCH In review Gerrit Renker 3 participants 7 messages Jun 9, 2007 – Jun 16, 2007 Networking

lore
Share & tools
lore
#0 lore raw

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

[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().

---
 net/dccp/ccids/ccid3.c |   23 ++++++++++-------------
 net/dccp/ccids/ccid3.h |    2 +-
 2 files changed, 11 insertions(+), 14 deletions(-)
net/dccp/ccids/ccid3.h +1 -1
@@ -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;
net/dccp/ccids/ccid3.c +10 -13
@@ -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) {
#1 lore raw
Quoted 7 lines · 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().

This looks OK but...

net/dccp/ccids/ccid3.c static inline void ccid3_update_send_int · line 128

Quoted 3 lines
net/dccp/ccids/ccid3.c +1 -1
-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

Signature · 3 lines
Web: http://wand.net.nz/~iam4/
Blog: http://iansblog.jandi.co.nz
WAND Network Research Group
#2 lore raw
In reply to 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.

#3 lore raw
In reply to Gerrit Renker
Quoted 23 lines · 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.

Quoted 8 lines

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

#4 lore raw
In reply to 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.

#5 lore raw
In reply to Gerrit Renker
Quoted 25 lines · 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 :-)
Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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,

Quoted 2 lines

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.

Quoted 3 lines

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!

Quoted 2 lines

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

#6 lore raw
In reply to 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.