Re: [RFC][PATCHES 0/7]: Reorganization of RX history patches
On 12/3/07, Arnaldo Carvalho de Melo [EMAIL PROTECTED] wrote: WARNING: After reading some messages from Ingo Molnar on lkml I think we should really trim the number of lists we use for kernel development. And since I moved back to using mutt for reading e-mails, something I should have never, ever stopped doing, I guess we should move the DCCP discussions to netdev, where we hopefully can get more people interested and reviewing the work we do, so please consider moving DCCP discussion to [EMAIL PROTECTED], where lots of smart networking folks are present and can help our efforts on turning RFCs to code. I (and others too) don't necessarily have time to read netdev so would vote on keeping dccp. I would totally agree to making sure that cross-post to netdev as well as dccp. Ian - To unsubscribe from this list: send the line unsubscribe dccp in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] [CCID3]: Hook up with new RX history interface
In addition, it makes two corrections too the code:
1. The receiver of a half-connection does not set window counter values;
only the sender sets window counters [RFC 4342, sections 5 and 8.1].
2. The computation of X_recv does currently not conform to TFRC/RFC 3448,
since this specification requires that X_recv be computed over the last
R_m seconds (sec. 6.2). The patch tackles this problem as it
- explicitly distinguishes the types of feedback (using an enum);
- uses previous value of X_recv when sending feedback due to a parameter
change;
- makes all state changes local to ccid3_hc_tx_packet_recv;
- assigns feedback type according to incident (previously only used flag
`do_feedback').
Further and detailed information is at
http://www.erg.abdn.ac.uk/users/gerrit/dccp/notes/ccid3_packet_reception/#8._Computing_X_recv_
Signed-off-by: Gerrit Renker [EMAIL PROTECTED]
Signed-off-by: Ian McDonald [EMAIL PROTECTED]
---
net/dccp/ccids/ccid3.c | 292 ++--
net/dccp/ccids/ccid3.h | 38 ---
2 files changed, 102 insertions(+), 228 deletions(-)
diff --git a/net/dccp/ccids/ccid3.c b/net/dccp/ccids/ccid3.c
index 5dea690..ef243dc 100644
--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -1,6 +1,7 @@
/*
* net/dccp/ccids/ccid3.c
*
+ * Copyright (c) 2007 The University of Aberdeen, Scotland, UK
* Copyright (c) 2005-7 The University of Waikato, Hamilton, New Zealand.
* Copyright (c) 2005-7 Ian McDonald [EMAIL PROTECTED]
*
@@ -49,8 +50,6 @@ static int ccid3_debug;
#define ccid3_pr_debug(format, a...)
#endif
-static struct dccp_rx_hist *ccid3_rx_hist;
-
/*
* Transmitter Half-Connection Routines
*/
@@ -675,52 +674,53 @@ static inline void ccid3_hc_rx_update_s(struct
ccid3_hc_rx_sock *hcrx, int len)
hcrx-ccid3hcrx_s = tfrc_ewma(hcrx-ccid3hcrx_s, len, 9);
}
-static void ccid3_hc_rx_send_feedback(struct sock *sk)
+static void ccid3_hc_rx_send_feedback(struct sock *sk, struct sk_buff *skb,
+ enum ccid3_fback_type fbtype)
{
struct ccid3_hc_rx_sock *hcrx = ccid3_hc_rx_sk(sk);
struct dccp_sock *dp = dccp_sk(sk);
- struct dccp_rx_hist_entry *packet;
- ktime_t now;
- suseconds_t delta;
-
- ccid3_pr_debug(%s(%p) - entry \n, dccp_role(sk), sk);
+ ktime_t now = ktime_get_real();
+ s64 delta = 0;
- now = ktime_get_real();
+ if (unlikely(hcrx-ccid3hcrx_state == TFRC_RSTATE_TERM))
+ return;
- switch (hcrx-ccid3hcrx_state) {
- case TFRC_RSTATE_NO_DATA:
+ switch (fbtype) {
+ case FBACK_INITIAL:
hcrx-ccid3hcrx_x_recv = 0;
+ hcrx-ccid3hcrx_pinv = ~0U; /* see RFC 4342, 8.5 */
break;
- case TFRC_RSTATE_DATA:
- delta = ktime_us_delta(now,
- hcrx-ccid3hcrx_tstamp_last_feedback);
- DCCP_BUG_ON(delta 0);
- hcrx-ccid3hcrx_x_recv =
- scaled_div32(hcrx-ccid3hcrx_bytes_recv, delta);
+ case FBACK_PARAM_CHANGE:
+ /*
+* When parameters change (new loss or p p_prev), we do not
+* have a reliable estimate for R_m of [RFC 3448, 6.2] and so
+* need to reuse the previous value of X_recv. However, when
+* X_recv was 0 (due to early loss), this would kill X down to
+* s/t_mbi (i.e. one packet in 64 seconds).
+* To avoid such drastic reduction, we approximate X_recv as
+* the number of bytes since last feedback.
+* This is a safe fallback, since X is bounded above by X_calc.
+*/
+ if (hcrx-ccid3hcrx_x_recv 0)
+ break;
+ /* fall through */
+ case FBACK_PERIODIC:
+ delta = ktime_us_delta(now, hcrx-ccid3hcrx_last_feedback);
+ if (delta = 0)
+ DCCP_BUG(delta (%ld) = 0, (long)delta);
+ else
+ hcrx-ccid3hcrx_x_recv =
+ scaled_div32(hcrx-ccid3hcrx_bytes_recv, delta);
break;
- case TFRC_RSTATE_TERM:
- DCCP_BUG(%s(%p) - Illegal state TERM, dccp_role(sk), sk);
- return;
- }
-
- packet = dccp_rx_hist_find_data_packet(hcrx-ccid3hcrx_hist);
- if (unlikely(packet == NULL)) {
- DCCP_WARN(%s(%p), no data packet in history!\n,
- dccp_role(sk), sk);
+ default:
return;
}
+ ccid3_pr_debug(Interval %ldusec, X_recv=%u, 1/p=%u\n, (long)delta,
+ hcrx-ccid3hcrx_x_recv, hcrx-ccid3hcrx_pinv);
- hcrx-ccid3hcrx_tstamp_last_feedback = now;
- hcrx-ccid3hcrx_ccval_last_counter = packet-dccphrx_ccval;
- hcrx-ccid3hcrx_bytes_recv
[PATCH 5/6] [TFRC]: Ringbuffer to track loss interval history
A ringbuffer-based implementation of loss interval history is easier to
maintain, allocate, and update.
Details:
* access to the Loss Interval Records via macro wrappers (with safety checks);
* simplified, on-demand allocation of entries (no extra memory consumption on
lossless links); cache allocation is local to the module / exported as
service;
* provision of RFC-compliant algorithm to re-compute average loss interval;
* provision of comprehensive, new loss detection algorithm
- support for all cases of loss, including re-ordered/duplicate packets;
- waiting for NDUPACK=3 packets to fill the hole;
- updating loss records when a late-arriving packet fills a hole.
Signed-off-by: Gerrit Renker [EMAIL PROTECTED]
Signed-off-by: Ian McDonald [EMAIL PROTECTED]
---
net/dccp/ccids/lib/loss_interval.c | 158 +-
net/dccp/ccids/lib/loss_interval.h | 58 +++-
net/dccp/ccids/lib/packet_history.c | 183 +++
net/dccp/ccids/lib/packet_history.h |4 +
net/dccp/ccids/lib/tfrc.h |3 +
5 files changed, 400 insertions(+), 6 deletions(-)
diff --git a/net/dccp/ccids/lib/loss_interval.c
b/net/dccp/ccids/lib/loss_interval.c
index 0ebdc67..cde5c7f 100644
--- a/net/dccp/ccids/lib/loss_interval.c
+++ b/net/dccp/ccids/lib/loss_interval.c
@@ -1,6 +1,7 @@
/*
* net/dccp/ccids/lib/loss_interval.c
*
+ * Copyright (c) 2007 The University of Aberdeen, Scotland, UK
* Copyright (c) 2005-7 The University of Waikato, Hamilton, New Zealand.
* Copyright (c) 2005-7 Ian McDonald [EMAIL PROTECTED]
* Copyright (c) 2005 Arnaldo Carvalho de Melo [EMAIL PROTECTED]
@@ -10,12 +11,7 @@
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*/
-
-#include linux/module.h
#include net/sock.h
-#include ../../dccp.h
-#include loss_interval.h
-#include packet_history.h
#include tfrc.h
#define DCCP_LI_HIST_IVAL_F_LENGTH 8
@@ -27,6 +23,51 @@ struct dccp_li_hist_entry {
u32 dccplih_interval;
};
+static struct kmem_cache *tfrc_lh_slab __read_mostly;
+/* Loss Interval weights from [RFC 3448, 5.4], scaled by 10 */
+static const int tfrc_lh_weights[NINTERVAL] = { 10, 10, 10, 10, 8, 6, 4, 2 };
+
+/*
+ * Access macros: These require that at least one entry is present in lh,
+ * and implement array semantics (0 is first, n-1 is the last of n entries).
+ */
+#define __lih_index(lh, n) LIH_INDEX((lh)-counter - (n) - 1)
+#define __lih_entry(lh, n) (lh)-ring[__lih_index(lh, n)]
+#define __curr_entry(lh) (lh)-ring[LIH_INDEX((lh)-counter - 1)]
+#define __next_entry(lh) (lh)-ring[LIH_INDEX((lh)-counter)]
+
+/* given i with 0 = i = k, return I_i as per the rfc3448bis notation */
+static inline u32 tfrc_lh_get_interval(struct tfrc_loss_hist *lh, u8 i)
+{
+ BUG_ON(i = lh-counter);
+ return __lih_entry(lh, i)-li_length;
+}
+
+static inline struct tfrc_loss_interval *tfrc_lh_peek(struct tfrc_loss_hist
*lh)
+{
+ return lh-counter ? __curr_entry(lh) : NULL;
+}
+
+/*
+ * On-demand allocation and de-allocation of entries
+ */
+static struct tfrc_loss_interval *tfrc_lh_demand_next(struct tfrc_loss_hist
*lh)
+{
+ if (__next_entry(lh) == NULL)
+ __next_entry(lh) = kmem_cache_alloc(tfrc_lh_slab, GFP_ATOMIC);
+
+ return __next_entry(lh);
+}
+
+void tfrc_lh_cleanup(struct tfrc_loss_hist *lh)
+{
+ if (tfrc_lh_is_initialised(lh))
+ for (lh-counter = 0; lh-counter LIH_SIZE; lh-counter++)
+ if (__next_entry(lh) != NULL)
+ kmem_cache_free(tfrc_lh_slab, __next_entry(lh));
+}
+EXPORT_SYMBOL_GPL(tfrc_lh_cleanup);
+
static struct kmem_cache *dccp_li_cachep __read_mostly;
static inline struct dccp_li_hist_entry *dccp_li_hist_entry_new(const gfp_t
prio)
@@ -98,6 +139,65 @@ u32 dccp_li_hist_calc_i_mean(struct list_head *list)
EXPORT_SYMBOL_GPL(dccp_li_hist_calc_i_mean);
+static void tfrc_lh_calc_i_mean(struct tfrc_loss_hist *lh)
+{
+ u32 i_i, i_tot0 = 0, i_tot1 = 0, w_tot = 0;
+ int i, k = tfrc_lh_length(lh) - 1; /* k is as in rfc3448bis, 5.4 */
+
+ for (i=0; i = k; i++) {
+ i_i = tfrc_lh_get_interval(lh, i);
+
+ if (i k) {
+ i_tot0 += i_i * tfrc_lh_weights[i];
+ w_tot += tfrc_lh_weights[i];
+ }
+ if (i 0)
+ i_tot1 += i_i * tfrc_lh_weights[i-1];
+ }
+
+ BUG_ON(w_tot == 0);
+ lh-i_mean = max(i_tot0, i_tot1) / w_tot;
+}
+
+/**
+ * tfrc_lh_update_i_mean - Update the `open' loss interval I_0
+ * For recomputing p: returns `true' if p p_prev = 1/p 1/p_prev
+ */
+u8 tfrc_lh_update_i_mean(struct tfrc_loss_hist *lh, struct sk_buff *skb)
+{
+ struct tfrc_loss_interval *cur = tfrc_lh_peek(lh);
+ u32 old_i_mean = lh-i_mean;
+ s64 length;
+
+
Re: [RFC][PATCHES 0/7]: Reorganization of RX history patches
Em Mon, Dec 03, 2007 at 08:35:12AM +, Gerrit Renker escreveu:
Hi Arnaldo,
hank you for going through this. I have just backported your recent patches
of 2.6.25
to the DCCP/CCID4/Faster Restart test tree at
git://eden-feed.erg.abdn.ac.uk/dccp_exp {dccp,ccid4,dccp_fr}
as per subsequent message.
| do, so please consider moving DCCP discussion to [EMAIL PROTECTED],
| where lots of smart networking folks are present and can help our
efforts
| on turning RFCs to code.
Are you suggesting using netdev exclusively or in addition to [EMAIL
PROTECTED]
Well, since at least one person that has contributed significantly in
the past has said he can't cope with traffic on netdev, we can CC
[EMAIL PROTECTED]
| Please take a look at this patch series where I reorganized your work
on the new
| TFRC rx history handling code. I'll wait for your considerations and then
do as many
| interactions as reasonable to get your work merged.
|
| It should be completely equivalent, plus some fixes and optimizations,
such as:
It will be necessary to address these points one-by-one. Before diving into
making
fixes and `optimisations', have you tested your code? The patches you are
referring to
I have performed basic tests, and when doing so noticed a bug in
inet_diag, that I commented with Herbert Xu and he has since provided a
fix.
have been posted and re-posted for a period of over 9 months on [EMAIL
PROTECTED], and
Being posted and re-posted does not guarantee that the patch is OK or
that is in a form that is acceptable to all tree maintainers. DaveM is
subscribed to [EMAIL PROTECTED] and could have picked this if he had the time to
do the review. I didn't, but now I'm trying to spend my time on
reviewing your patches and in the process doing modifications I find
appropriate while trying hard not to introduce bugs in your hard work
to get it merged.
there are regression tests which show that this code improves on the existing
Linux
implementation. These are labelled as `test tree' on
http://www.linux-foundation.org/en/Net:DCCP_Testing#Regression_testing
So if you are making changes to the code, I would like to ask if you have run
similar
regression tests, to avoid having to step back later.
Fair enough, I will do that before asking Herbert or Dave to pull from
my tree.
| . The code that allocates the RX ring deals with failures when one of the
entries in
| the ring buffer is not successfully allocated, the original code was
leaking the
| successfully allocated entries.
Sorry for not point out exactly this, here it goes:
Your original patch:
+int tfrc_rx_hist_init(struct tfrc_rx_hist *h)
+{
+ int i;
+
+ for (i = 0; i = NDUPACK; i++) {
+ h-ring[i] = kmem_cache_alloc(tfrc_rxh_cache, GFP_ATOMIC);
+ if (h-ring[i] == NULL)
+ return 1;
+ }
+ h-loss_count = 0;
+ h-loss_start = 0;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tfrc_rx_hist_init);
Then, in ccid3_hc_rx_init you do:
static int ccid3_hc_rx_init(struct ccid *ccid, struct sock *sk)
{
struct ccid3_hc_rx_sock *hcrx = ccid_priv(ccid);
ccid3_pr_debug(entry\n);
hcrx-ccid3hcrx_state = TFRC_RSTATE_NO_DATA;
tfrc_lh_init(hcrx-ccid3hcrx_li_hist);
return tfrc_rx_hist_init(hcrx-ccid3hcrx_hist);
}
So if tfrc_rx_hist_init fail to allocate, say, the last entry in the
ring, it will return 1, and from looking at how you initialize
h-loss_{count,start} to zero I assumed that the whole tfrc_rx_hist
is not zeroed when tfrc_rx_hist_init is called, so one can also assume
that the ring entries are not initialized to NULL and that if the
error recovery is to assume that later on tfrc_rx_hist_cleanup is called
we would not have a leak, but an OOPS when tfrc_rx_hist_cleanup tries
to call kfree_cache_free on the uninitialized ring entries.
But if you look at ccid_new(), that calls ccid3_hc_rx_init(), you'll see
that when the ccid rx or hx routine fails, it just frees the
struct ccid with the area where h-ring is, so, what was not cleaned up
by the ccid init routine is effectively forgot, leaked.
I first did the cleanup at tfrc_rx_hist_init (that I renamed to
tfrc_rx_hist_alloc, since it doesn't just initializes things, but
allocates entries from slab), but then I just made the rx history slab
have arrays of tfrc_rx_hist_entry objects, not individual ones as your
code always allocates them as arrays.
| . We do just one allocation for the ring buffer, as the number of entries
is fixed we
| should just do one allocation and not TFRC_NDUPACK times.
Will look at the first point in the patch; with regard to the second point I
agree, this
will make the code simpler, which is good.
good
| . I haven't checked if all the code was commited, as I tried to introduce
just what was
| immediatelly used, probably we'll need to do some changes when working on
the merge
| of
Re: [Announce]: Test tree up{dated,loaded}
2007/12/3, Gerrit Renker [EMAIL PROTECTED]:
I have just backported the latest changes that Arnaldo put in the 2.6.25
tree to the test tree. While it seems correct, I have not run exhaustive
tests, but will run a few.
I have also updated the CCID4/Faster Restart branches with regard to
these changes and would like to know if people are actually using these
branches, as it takes time to keep track of what is going on.
The interdiff for the CCID4 tree is below (there was just one hunk which
failed in the interdiff; if in doubt please check out the code). The
updates to CCID3 are analogous.
--- b/net/dccp/ccids/ccid4.c
+++ b/net/dccp/ccids/ccid4.c
@@ -56,8 +56,6 @@
#define ccid4_pr_debug(format, a...)
#endif
-DECLARE_TFRC_TX_CACHE(ccid4_tx_hist);
-
static void ccid4_hc_tx_set_state(struct sock *sk,
enum tfrc_hc_tx_states state)
{
@@ -355,7 +353,7 @@
{
struct tfrc_hc_tx_sock *hctx = tfrc_hc_tx_sk(sk);
struct tfrc_options_received *opt_recv;
- ktime_t t_send, now;
+ ktime_t now;
unsigned long t_nfb;
u32 pinv, r_sample;
@@ -363,24 +361,26 @@
if (!(DCCP_SKB_CB(skb)-dccpd_type == DCCP_PKT_ACK ||
DCCP_SKB_CB(skb)-dccpd_type == DCCP_PKT_DATAACK))
return;
-
/* ... and only in the established state */
- switch (hctx-tfrchctx_state) {
- case TFRC_SSTATE_NO_FBACK: /* fall through */
- case TFRC_SSTATE_FBACK: break;
- default:return;
- }
+ if (hctx-tfrchctx_state != TFRC_SSTATE_FBACK
+ hctx-tfrchctx_state != TFRC_SSTATE_NO_FBACK)
+ return;
- /* estimate RTT from history if ACK number is valid */
- if (! tfrc_tx_hist_when(t_send, hctx-tfrchctx_hist,
- DCCP_SKB_CB(skb)-dccpd_ack_seq)) {
+ opt_recv = hctx-tfrchctx_options_received;
+ now = ktime_get_real();
+
+ /* Estimate RTT from history if ACK number is valid */
+ r_sample = tfrc_tx_hist_rtt(hctx-tfrchctx_hist,
+ DCCP_SKB_CB(skb)-dccpd_ack_seq, now);
+ if (r_sample == 0) {
DCCP_WARN(%s(%p): %s with bogus ACK-%llu
, dccp_role(sk), sk,
dccp_packet_name(DCCP_SKB_CB(skb)-dccpd_type),
(unsigned long
long)DCCP_SKB_CB(skb)-dccpd_ack_seq);
return;
}
-
- opt_recv = hctx-tfrchctx_options_received;
+ /* Validate new RTT sample and update moving average */
+ r_sample = dccp_sample_rtt(sk, r_sample);
+ hctx-tfrchctx_rtt = tfrc_ewma(hctx-tfrchctx_rtt, r_sample, 9);
/* Update receive rate in units of 64 * bytes/second */
hctx-tfrchctx_x_recv = opt_recv-tfrcor_receive_rate;
@@ -394,13 +394,6 @@
hctx-tfrchctx_p = scaled_div(1, pinv);
/*
-* Calculate new RTT sample and update moving average
-*/
- now = ktime_get_real();
- r_sample = dccp_sample_rtt(sk, ktime_us_delta(now, t_send));
- hctx-tfrchctx_rtt = tfrc_ewma(hctx-tfrchctx_rtt, r_sample, 9);
-
- /*
* Update allowed sending rate X as per draft rfc3448bis-00,
4.2/3
*/
if (hctx-tfrchctx_state == TFRC_SSTATE_NO_FBACK) {
@@ -440,7 +433,7 @@
(unsigned)(hctx-tfrchctx_x_recv 6),
(unsigned)(hctx-tfrchctx_x 6));
- /* unschedule no feedback timer */
+ /* unschedule nofeedback timer */
sk_stop_timer(sk, hctx-tfrchctx_no_feedback_timer);
/*
@@ -541,7 +534,7 @@
struct tfrc_hc_tx_sock *hctx = ccid_priv(ccid);
hctx-tfrchctx_state = TFRC_SSTATE_NO_SENT;
- tfrc_tx_hist_init(hctx-tfrchctx_hist, ccid4_tx_hist);
+ hctx-tfrchctx_hist = NULL;
setup_timer(hctx-tfrchctx_no_feedback_timer,
ccid4_hc_tx_no_feedback_timer, (unsigned long)sk);
@@ -555,8 +548,7 @@
ccid4_hc_tx_set_state(sk, TFRC_SSTATE_TERM);
sk_stop_timer(sk, hctx-tfrchctx_no_feedback_timer);
- /* Empty packet history */
- tfrc_tx_hist_cleanup(hctx-tfrchctx_hist);
+ tfrc_tx_hist_purge(hctx-tfrchctx_hist);
}
static void ccid4_hc_tx_get_info(struct sock *sk, struct tcp_info *info)
@@ -884,25 +876,13 @@
static __init int ccid4_module_init(void)
{
- int rc = -ENOBUFS;
-
- if (tfrc_tx_cache_init(ccid4_tx_hist, ccid4))
- goto out;
-
- rc = ccid_register(ccid4);
- if (rc != 0)
- tfrc_tx_cache_cleanup(ccid4_tx_hist);
-out:
- return rc;
+ return ccid_register(ccid4);
}
module_init(ccid4_module_init);
static __exit void ccid4_module_exit(void)
{
ccid_unregister(ccid4);
-
- if (ccid4_tx_hist != NULL)
- tfrc_tx_cache_cleanup(ccid4_tx_hist);
}
Re: [RFC][PATCHES 0/7]: Reorganization of RX history patches
Em Mon, Dec 03, 2007 at 01:49:47PM +, Gerrit Renker escreveu:
| Are you suggesting using netdev exclusively or in addition to [EMAIL
PROTECTED]
|
| Well, since at least one person that has contributed significantly in
| the past has said he can't cope with traffic on netdev, we can CC
| dccp@vger.kernel.org
I have a similar problem with the traffic but agree and will copy as well.
| have been posted and re-posted for a period of over 9 months on [EMAIL
PROTECTED], and
|
| Being posted and re-posted does not guarantee that the patch is OK or
| that is in a form that is acceptable to all tree maintainers.
With the first point I couldn't agree more, but this is not really what I
meant - the point
was that despite posting and re-posting there was often silence. And now
there is feedback,
in form of a patchset made by you; and all that I am asking for is just to be
given the time to
look that through. Last time a RFC patch appeared at 3pm and was checked in
the same evening
(only found out next morning).
I was too optimistic about that one, feeling that it was safe, sorry
about that, will avoid doing that in the future.
With your experience and long background as a maintainer maybe you expect
quicker turn-around
times, but I also had waited patiently until you had had a chance to review
the stuff I sent.
Agreed, my bad, will be more patient with my side as you've been with
yours :-)
| | . The code that allocates the RX ring deals with failures when one of
the entries in
| | the ring buffer is not successfully allocated, the original code was
leaking the
| | successfully allocated entries.
|
| Sorry for not point out exactly this, here it goes:
|
| Your original patch:
|
| +int tfrc_rx_hist_init(struct tfrc_rx_hist *h)
| +{
| + int i;
| +
| + for (i = 0; i = NDUPACK; i++) {
| + h-ring[i] = kmem_cache_alloc(tfrc_rxh_cache, GFP_ATOMIC);
| + if (h-ring[i] == NULL)
| + return 1;
| + }
| + h-loss_count = 0;
| + h-loss_start = 0;
| + return 0;
| +}
| +EXPORT_SYMBOL_GPL(tfrc_rx_hist_init);
|
I expected this and it actually was very clear from your original message. I
fully back up
your solution in this point, see below. But my question above was rather: are
there any
other bugs rather than the above leakage, which is what the previous email
seemed to indicate?
With regard to your solution - you are using
int tfrc_rx_hist_alloc(struct tfrc_rx_hist *h)
{
h-ring = kmem_cache_alloc(tfrc_rx_hist_slab, GFP_ATOMIC);
h-loss_count = h-loss_start = 0;
return h-ring == NULL;
}
which is better not only for one but for two reasons. It solves the leakage
and in addition makes
the entire code simpler. Fully agreed.
good
|
| | . I haven't checked if all the code was commited, as I tried to
introduce just what was
| | immediatelly used, probably we'll need to do some changes when
working on the merge
| | of your loss intervals code.
| Sorry I don't understand this point.
|
| Let me check now and tell you for sure:
|
| tfrc_rx_hist_delta_seqno and tfrc_rx_hist_swap were not included, as
| they were not used, we should introduce them later, when getting to the
| working on the loss interval code.
Ah thanks, that was really not clear. Just beginning to work through the set.
great
|static void ccid3_hc_rx_packet_recv(struct sock *sk, struct sk_buff
*skb)
|{
|// ...
|u32 sample, payload_size = skb-len - dccp_hdr(skb)-dccph_doff
* 4;
|
|if (unlikely(hcrx-ccid3hcrx_state == TFRC_RSTATE_NO_DATA)) {
|if (is_data_packet) {
|do_feedback = FBACK_INITIAL;
|ccid3_hc_rx_set_state(sk, TFRC_RSTATE_DATA);
|ccid3_hc_rx_update_s(hcrx, payload_size);
|}
|goto update_records;
|}
|
| == Non-data packets are ignored for the purposes of computing s (this is
in the RFC),
| consequently update_s() is only called for data packets; using the
two following
| functions:
|
|
|static inline u32 tfrc_ewma(const u32 avg, const u32 newval, const u8
weight)
|{
|return avg ? (weight * avg + (10 - weight) * newval) / 10 :
newval;
|}
|
| I hadn't considered that tfrc_ewma would for every packet check if the
| avg was 0 and I find it suboptimal now that I look at it, we are just
| feeding data packets, no?
Yes exactly, only data packets are used for s.
|static inline void ccid3_hc_rx_update_s(struct ccid3_hc_rx_sock *hcrx,
int len)
|{
|if (likely(len 0))/* don't update on empty packets (e.g.
ACKs) */
|hcrx-ccid3hcrx_s = tfrc_ewma(hcrx-ccid3hcrx_s, len,
9);
|}
|
| And we
Re: [RFC][PATCHES 0/7]: Reorganization of RX history patches
| | static inline void ccid3_hc_rx_update_s(struct ccid3_hc_rx_sock
*hcrx, int len)
| | {
| | if (likely(len 0))/* don't update on empty
packets (e.g. ACKs) */
| | hcrx-ccid3hcrx_s =
tfrc_ewma(hcrx-ccid3hcrx_s, len, 9);
| | }
| |
| | And we also just do test for len 0 in update_s, that looks like
| | also excessive, no?
| Hm, I think we need to make it robust against API bugs and/or zero-sized
| data packets. The check `len 0' may seem redundant but it catches such
| a condition. For a moving average an accidental zero value will have
| quite an impact on the overall value. In CCID3 it is
|
| x_new = 0.9 * x_old + 0.1 * len
|
| So if len is accidentally 0 where it should not be, then each time the
| moving average is reduced to 90%.
|
| So we must make it a BUG_ON, not something that is to be always present.
|
I think it should be a warning condition since it can be triggered when
the remote party sends zero-sized packets. It may be good to log this
into the syslog to warn about possibly misbehaving apps/peers/remote
stacks.
| As a comparison - the entire patch set took about a full month to do.
| But that meant I am reasonably sure the algorithm is sound and can cope
| with problematic conditions.
|
| And from what I saw so far that is my impression too, if you look at
| what I'm doing it is:
|
| 1. go thru the whole patch trying to understand hunk by hunk
You are doing a great job - in particular as it really is a lot of material.
| 2. do consistency changes (add namespace prefixes)
| 3. reorganize the code to look more like what is already there, we
|both have different backgrounds and tastes about how code should
|be written, so its only normal that if we want to keep the code
|consistent, and I want that, I jump into things I think should be
|reworded, while trying to keep the algorithm expressed by you.
|
Agree, that is not always easy to get right. I try to stick as close as
possible to existing conventions but of course that is my
interpretation, so I am already anticipating such changes/comments here.
| think about further automatization on regression testing.
|
If it is of any use, some scripts and setups are at the bottom of the page at
http://www.erg.abdn.ac.uk/users/gerrit/dccp/testing_dccp/
-
To unsubscribe from this list: send the line unsubscribe dccp in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
