Re: [PATCH 2/5] Implement loss counting on TFRC-SP receiver

[Ivo Calado <ivocalado@embedded.ufcg.edu.br>]

In the same way, my comments follow below


>                s64 len = dccp_delta_seqno(cur->li_seqno, cong_evt_seqno);
>                if ((len <= 0) ||
>                    (!tfrc_lh_closed_check(cur, cong_evt->tfrchrx_ccval))) {
> +                       cur->li_losses += rh->num_losses;
>                        return false;
>                }
> This has a multiplicative effect, since rh->num_losses is added to cur->li_losses
> each time the condition is evaluated. E.g. if 3 times in a row reordered (earlier)
> sequence numbers arrive, or if the CCvals do not differ (high-speed networks),
> we end up with 3 * rh->num_losses, which can't be correct.
>
>


The following code would be correct then?

              if ((len <= 0) ||
                  (!tfrc_lh_closed_check(cur, cong_evt->tfrchrx_ccval))) {
+                       cur->li_losses += rh->num_losses;
+                       rh->num_losses  = 0;
                      return false;
With this change I suppose the could be fixed. With that, the
rh->num_losses couldn't added twice. Am I correct?




>
> --- dccp_tree_work4.orig/net/dccp/ccids/lib/packet_history_sp.c
> +++ dccp_tree_work4/net/dccp/ccids/lib/packet_history_sp.c
> @@ -244,6 +244,7 @@
>                h->loss_count = 3;
>                tfrc_sp_rx_hist_entry_from_skb(tfrc_rx_hist_entry(h, 3),
>                                               skb, n3);
> +               h->num_losses = dccp_loss_count(s2, s3, n3);
>                return 1;
>        }
> This only measures the gap between s2 and s3, but the "hole" starts at s0,
> so it would need to be dccp_loss_count(s0, s3, n3). Algorithm is documented at
> http://www.erg.abdn.ac.uk/users/gerrit/dccp/notes/\
> ccid3/ccid3_packet_reception/loss_detection/loss_detection_algorithm_notes.txt
>

<snip>

>  }
> Here it is also between s0 and s2, not between s0 and s3. It is case VI(c.3).
>
> However, the above still is a crude approximation, since it only measures between
> the last sequence number received before the loss and the third sequence number
> after the loss. It would be better to either
>
>  * use the first sequence number after the loss (this can be s1, s2, or s3) or
>  * check if there are more holes between the first/second and the second/third
>   sequence numbers after the loss.
>
> The second option would be the correct one, it should also take the NDP counts
> of each gap into account. And already we have a fairly complicated algorithm.
>



I'll study loss_detection_algorithm_notes.txt and correct the code.
But I have one question, that i don't know if is already answered by
the documentation:
Further holes, between the the first and third packet received after
the hole are accounted only in future calls to the function, right?
Because the receiver needs to receive more packets to confirm loss,
right?
So, it's really necessary to look for other holes after the loss? Will
not this other holes be identified as losses in future?



> Another observation is that this function is only called in packet_history_sp.c,
> and only in __two_after_loss(), so that dccp_loss_count() could be made static,
> and without the need for the WARN_ON (see below), since in all above cases it is
> ensured that the first sequence number argument is "before" the second one.
>

Okay.
>
> --- dccp_tree_work4.orig/net/dccp/ccids/lib/packet_history_sp.h
> +++ dccp_tree_work4/net/dccp/ccids/lib/packet_history_sp.h
> @@ -113,6 +113,7 @@
>        u32                       packet_size,
>                                  bytes_recvd;
>        ktime_t                   bytes_start;
> +       u8                        num_losses;
>  };
> No more than 255 losses? NDP count option has space for up to 6 bytes, i.e. 2^48-1.
> Suggest u64 for consistency with the other parts.
>

Okay.


>
> --- dccp_tree_work4.orig/net/dccp/dccp.h
> +++ dccp_tree_work4/net/dccp/dccp.h
> @@ -168,6 +168,21 @@
>        return (u64)delta <= ndp + 1;

<snip>

> But then dccp_loss_free reduces to a specialisation of the above:
> bool dccp_loss_free(const u64 s1, const u64 s2, const u64 ndp)
> {
>        return dccp_loss_count(s1, s2, ndp) == 0;
> }
>
> But please see above -- the function needs to be called for each hole in a row.
>

Thanks for correcting the calculation for me!

-- 
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.