: tfrc_tx_hist_rtt
RFC 1/1 : tfrc_tx_hist_rtt
Hi Gerrit,
Coming back to hiding the TX history data structures, take a
look at what I'm commiting to my tree:
1. struct tfrc_tx_hist_entry is completely hidden in net/dccp/ccids/lib/packet_history.c
2. I found tfrc_tx_hist_when confusing and returning two results, if it had found the ackno in the history and the timestamp, so I introduced tfrc_tx_hist_rtt instead, that returns 0 if the ackno was not found.
Does this looks reasonable?
- Arnaldo
net/dccp/ccids/ccid3.c +7 -12
@@ -399,7 +399,6 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb) {struct ccid3_hc_tx_sock *hctx =ccid3_hc_tx_sk (sk);struct ccid3_options_received *opt_recv; -struct tfrc_tx_hist_entry *packet;ktime_t now;unsigned long t_nfb;u32 pinv, r_sample;
@@ -414,19 +413,17 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)switch (hctx->ccid3hctx_state) {case TFRC_SSTATE_NO_FBACK :case TFRC_SSTATE_FBACK : + now =ktime_get_real (); +/* estimate RTT from history if ACK number is valid */ - packet =tfrc_tx_hist_find_entry (hctx->ccid3hctx_hist, -DCCP_SKB_CB (skb)->dccpd_ack_seq); -if (packet =NULL ) { + r_sample =tfrc_tx_hist_rtt (hctx->ccid3hctx_hist, +DCCP_SKB_CB (skb)->dccpd_ack_seq, now); +if (r_sample =0 ) {DCCP_WARN ("%s(%p): %s with bogus ACK-%llu\n" ,dccp_role (sk), sk,dccp_packet_name (DCCP_SKB_CB (skb)->dccpd_type), (unsigned long long )DCCP_SKB_CB (skb)->dccpd_ack_seq);return ; } -/* -* Garbage-collect older (irrelevant) entries -*/ -tfrc_tx_hist_purge (&packet->next);/* Update receive rate in units of 64 * bytes/second */ hctx->ccid3hctx_x_recv = opt_recv->ccid3or_receive_rate;
@@ -438,12 +435,10 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb) hctx->ccid3hctx_p =0 ; else /* can not exceed 100% */ hctx->ccid3hctx_p = 1000000 / pinv; - - now =ktime_get_real ();/* -* Calculate new RTT sample and update moving average +* Validate new RTT sample and update moving average */ - r_sample =dccp_sample_rtt (sk,ktime_us_delta (now, packet->stamp)); + r_sample =dccp_sample_rtt (sk, r_sample); hctx->ccid3hctx_rtt =tfrc_ewma (hctx->ccid3hctx_rtt, r_sample,9 );if (hctx->ccid3hctx_state =TFRC_SSTATE_NO_FBACK ) {
net/dccp/ccids/lib/packet_history.c +31 -2
@@ -39,12 +39,24 @@#include <linux/string.h> #include "packet_history.h" +/** +* tfrc_tx_hist_entry - Simple singly-linked TX history list +* @next: next oldest entry (LIFO order) +* @seqno: sequence number of this entry +* @stamp: send time of packet with sequence number @seqno +*/ +struct tfrc_tx_hist_entry { +struct tfrc_tx_hist_entry *next; +u64 seqno; +ktime_t stamp; +}; +/* * Transmitter History Routines */ static struct kmem_cache *tfrc_tx_hist; -struct tfrc_tx_hist_entry * +static struct tfrc_tx_hist_entry *tfrc_tx_hist_find_entry (struct tfrc_tx_hist_entry *head,u64 seqno) {while (head !=NULL && head->seqno != seqno)
@@ -52,7 +64,6 @@ struct tfrc_tx_hist_entry *return head; } -EXPORT_SYMBOL_GPL (tfrc_tx_hist_find_entry);int tfrc_tx_hist_add (struct tfrc_tx_hist_entry **headp,u64 seqno) {
@@ -83,6 +94,24 @@ void tfrc_tx_hist_purge(struct tfrc_tx_hist_entry **headp) }EXPORT_SYMBOL_GPL (tfrc_tx_hist_purge); +u32 tfrc_tx_hist_rtt (struct tfrc_tx_hist_entry *head,const u64 seqno, +const ktime_t now) +{ +u32 rtt =0 ; +struct tfrc_tx_hist_entry *packet =tfrc_tx_hist_find_entry (head, seqno); + +if (packet !=NULL ) { + rtt =ktime_us_delta (now, packet->stamp); +/* +* Garbage-collect older (irrelevant) entries: +*/ +tfrc_tx_hist_purge (&packet->next); + } + +return rtt; +} +EXPORT_SYMBOL_GPL (tfrc_tx_hist_rtt); +/* * Receiver History Routines */
net/dccp/ccids/lib/packet_history.h +3 -14
@@ -48,23 +48,12 @@#define TFRC_WIN_COUNT_PER_RTT 4#define TFRC_WIN_COUNT_LIMIT 16 -/** -* tfrc_tx_hist_entry - Simple singly-linked TX history list -* @next: next oldest entry (LIFO order) -* @seqno: sequence number of this entry -* @stamp: send time of packet with sequence number @seqno -*/ -struct tfrc_tx_hist_entry { -struct tfrc_tx_hist_entry *next; -u64 seqno; -ktime_t stamp; -}; +struct tfrc_tx_hist_entry ;extern int tfrc_tx_hist_add (struct tfrc_tx_hist_entry **headp,u64 seqno);extern void tfrc_tx_hist_purge (struct tfrc_tx_hist_entry **headp); - -extern struct tfrc_tx_hist_entry * -tfrc_tx_hist_find_entry (struct tfrc_tx_hist_entry *head,u64 ackno); +extern u32 tfrc_tx_hist_rtt (struct tfrc_tx_hist_entry *head, +const u64 seqno,const ktime_t now);/* * Receiver History data structures and declarations
Sorry I only got this email today and it is a busy day, too.
The changes look good and are in general a further improvement on the code. I like the idea of hiding the internals of the list structure in the source file.
I wonder if one could go one step further and also take the timestamp directly when looking up the previous history sample, e.g.
u32 tfrc_tx_hist_rtt(struct tfrc_tx_hist_entry *head, const u64 seqno)
{
u32 rtt = 0;
struct tfrc_tx_hist_entry *packet = tfrc_tx_hist_find_entry(head, seqno);
if (packet != NULL) {
rtt = ktime_us_delta(ktime_get_real(), packet->stamp);
/*
* Garbage-collect older (irrelevant) entries:
*/
tfrc_tx_hist_purge(&packet->next);
}
return rtt;
}Just a suggestion.
Thanks for looking through this.
Em Fri, Nov 30, 2007 at 12:19:36PM +0000, Gerrit Renker escreveu:
Quoted 25 lines
Sorry I only got this email today and it is a busy day, too.
The changes look good and are in general a further improvement on the code. I like the idea of hiding the internals of the list structure in the source file.
I wonder if one could go one step further and also take the timestamp directly when looking up the previous history sample, e.g.
u32 tfrc_tx_hist_rtt(struct tfrc_tx_hist_entry *head, const u64 seqno) { u32 rtt = 0; struct tfrc_tx_hist_entry *packet = tfrc_tx_hist_find_entry(head, seqno); if (packet != NULL) { rtt = ktime_us_delta(ktime_get_real(), packet->stamp); /* * Garbage-collect older (irrelevant) entries: */ tfrc_tx_hist_purge(&packet->next); } return rtt; }
Quoted 1 line
Just a suggestion.
I thought about that, but ccid3_hc_tx_packet_recv needs a timestamp some lines below, when we reuse the ktime_get_real call.
- Arnaldo
| > I wonder if one could go one step further and also take the timestamp | > directly when looking up the previous history sample, e.g. | > <snip> | | I thought about that, but ccid3_hc_tx_packet_recv needs a timestamp some | lines below, when we reuse the ktime_get_real call. |
Oh, of course - that was real stupid of me. I think it is even in two occasions where a timestamp is needed, below in the same function.
I am really hoping that some time there is a robust RFC1323-like algorithm, this could solve all the RTT sampling for both CCID2 and CCID3 (also CCID4), and then there would be a RTT available in the main module (e.g. for better estimating the Close/CloseReq retransmission timeout).