The comments follow below
On Sun, Sep 13, 2009 at 14:28, Gerrit Renker <gerrit@xxxxxxxxxxxxxx> wrote:
> | Implement TFRC-SP calc of mean length of loss intervals accordingly to
> | section 3 of RFC 4828
> Sorry this also has problems.
>
> --- dccp_tree_work5.orig/net/dccp/ccids/lib/loss_interval_sp.c
> +++ dccp_tree_work5/net/dccp/ccids/lib/loss_interval_sp.c
> @@ -67,10 +67,11 @@
> }
> }
>
> -static void tfrc_sp_lh_calc_i_mean(struct tfrc_loss_hist *lh)
> +static void tfrc_sp_lh_calc_i_mean(struct tfrc_loss_hist *lh, __u8 curr_ccval)
> {
> 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 */
> + u32 losses;
>
> if (k <= 0)
> return;
> @@ -78,6 +79,15 @@
> for (i = 0; i <= k; i++) {
> i_i = tfrc_lh_get_interval(lh, i);
>
> + if (SUB16(curr_ccval,
> + tfrc_lh_get_loss_interval(lh, i)->li_ccval) <= 8) {
> +
> + losses = tfrc_lh_get_loss_interval(lh, i)->li_losses;
> +
> + if (losses > 0)
> + i_i = div64_u64(i_i, losses);
> + }
> +
> (Both 'i_i' and 'losses' are u32, so could write "i_i /= losses" instead.)
>
> However, this computation is done in the wrong place: here it is already too
> late. The N/K in RFC 4828 refers to entering each individual interval I_0, so
> the division (and the check whether this is a "short" interval) needs to be
> done when adding the interval, in tfrc_sp_lh_interval_add(). (There also exists
> a special rule for the open interval I_0, see RFC 5348, 5.3.)
>
Oh, all right. I understood it was wrong. I thought that the loss
interval needed to be less than 2 rtt old
but the correct is that it needs to be less than 2 rtt long, right?
> A second problem is a divide-by-zero condition encoded in the above: if
> i_i < losses, the result is 0, if all intervals are 0 then I_mean = 0, and
> thus p = 1/I_mean triggers a panic. In all other cases, the integer arithmetic
> has the effect of floor(N/K), i.e. it decreases the interval length.
>
> A way out (which does not fix the truncation problem) is to round up:
>
> if (losses > 0)
> i_i = DIV_ROUND_UP(i_i, losses);
>
> In fact, we have to do this, to avoid the divide-by-zero condition.
When the loss count (losses) could be greater than loss interval length (i_i)?
>
> The third problem is that the CCVal can be wrong, i.e. "less than 8" can
> also mean that it just wrapped around one or more times. But this is noted
> already in RFC 5622, it is a problem of the specification.
>
> @@ -87,6 +97,11 @@
> }
>
> lh->i_mean = max(i_tot0, i_tot1) / w_tot;
> + BUG_ON(w_tot == 0);
> + if (SUB16(curr_ccval, tfrc_lh_get_loss_interval(lh, 0)->li_ccval) > 8)
> + lh->i_mean = max(i_tot0, i_tot1) / w_tot;
> + else
> + lh->i_mean = i_tot1 / w_tot;
> }
> The line above the BUG_ON is probably a leftover. For the BUG_ON, this would
> be too late (division by zero).
> In fact, the BUG_ON is not needed, due to testing for k <= 0, see
> http://mirror.celinuxforum.org/gitstat//commit-detail.php?commit=eff253c4272cd2aac95ccff46d3d2e1a495f22b1
>
> @@ -127,7 +142,7 @@
> return;
>
> cur->li_length = len;
> - tfrc_sp_lh_calc_i_mean(lh);
> + tfrc_sp_lh_calc_i_mean(lh, dccp_hdr(skb)->dccph_ccval);
> }
> Should be division as per above.
>
> --- dccp_tree_work5.orig/net/dccp/ccids/lib/loss_interval_sp.h
> +++ dccp_tree_work5/net/dccp/ccids/lib/loss_interval_sp.h
> @@ -73,7 +73,8 @@
> extern bool tfrc_sp_lh_interval_add(struct tfrc_loss_hist *,
> struct tfrc_rx_hist *,
> u32 (*first_li)(struct sock *),
> - struct sock *);
> + struct sock *,
> + __u8 ccval);
> This function already has the ccval available, so could use it instead
> of just passing it through.
>
Thanks for observing these points!
--
Ivo Augusto Andrade Rocha Calado
MSc. Candidate
Embedded Systems and Pervasive Computing Lab - http://embedded.ufcg.edu.br
Systems and Computing Department - http://www.dsc.ufcg.edu.br
Electrical Engineering and Informatics Center - http://www.ceei.ufcg.edu.br
Federal University of Campina Grande - http://www.ufcg.edu.br
PGP: 0x03422935
Quidquid latine dictum sit, altum viditur.
--
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
