Quoting Ian McDonald:
| I'll have to have a think about this one some more as I spent a lot of
| time fixing these timing and I distinctly remember getting this one
| right!
Please relax: It is not my intention to embark on a game of "who is right here". I think
you have done a good job getting this code so far as it is, given the many and in parts
confusing documents that have to be read (for comparison: UDP, RFC 768, needs only 3 pages).
My intention is in making sure it meets the specifications and contribute where I can to fix things.
Regarding the patch: I have satisfied myself that what I am doing is correct,
I have added detailed explanations/illustrations so that others can validate
that it is indeed correct, and point out if I should have made an error.
| The timing is altered all over the code and it can't be fixed
| in one place in isolation (unless the whole code is simplified).
If you are saying that the other code depends on calculating t_ipi in this non-standard fashion,
then it is high time that the other code also gets fixed.
However, I don't believe so since t_ipi and t_delta are recalculated in only two places:
* in ccid3_hc_tx_packet_sent, state TFRC_SSTATE_FBACK:
timeval_sub_usecs(&hctx->ccid3hctx_t_nom,
hctx->ccid3hctx_t_ipi);
ccid3_calc_new_t_ipi(hctx);
ccid3_calc_new_delta(hctx);
timeval_add_usecs(&hctx->ccid3hctx_t_nom,
hctx->ccid3hctx_t_ipi);
* in ccid3_hc_tx_packet_recv, after updating X_calc:
/* Update sending rate */
ccid3_hc_tx_update_x(sk);
/* Update next send time */
timeval_sub_usecs(&hctx->ccid3hctx_t_nom,
hctx->ccid3hctx_t_ipi);
ccid3_calc_new_t_ipi(hctx);
timeval_add_usecs(&hctx->ccid3hctx_t_nom,
hctx->ccid3hctx_t_ipi);
ccid3_calc_new_delta(hctx);
==> Since these bits are so similar, I would actually suggest to accommodate them in
one function, since:
--t_ipi changes only if one of `s' or X_calc changes
--if `s' / X_calc remain unchanged after feedback, t_ipi remains as before
--t_delta depends on t_ipi
--the calculation of t_nom depends on t_ipi (as in the above snippets)
Conclusion:
-----------
I remain unconvinced that the code below violates anything: it is a fix. If there are bits
of the code you think depend on this then these too should be fixed.t_ipi by definition only
depends on X_calc, s, and the initial settings from RFC 3448, 4.6; it should not have or satisfy
other dependencies. If this is the case, please point out these things or submit a patch.
I am willing to help fixing these issues and open to argumentation, but I am not willing to
retract the patch unless there are sound arguments against it.
Gerrit
|
| I'll study this some more when I can...
|
| On 11/22/06, Gerrit Renker <gerrit@xxxxxxxxxxxxxx> wrote:
| > [CCID 3]: Fix calculation of t_ipi time of scheduled transmission
| >
| > Problem:
| > --------
| > Currently packet transmissions are not scheduled according to [RFC 3448, 4.6] and
| > will, in the worst case, be sent later than necessary. If scheduled transmission
| > requires using a delay, the current delay value is also larger as necessary.
| >
| > Solution:
| > ---------
| > This patch fixes time and delay calculation for scheduling CCID 3 packets.
| >
| >
| > Signed-off-by: Gerrit Renker <gerrit@xxxxxxxxxxxxxx>
| >
| >
| >
| > D e t a i l e d D e r i v a t i o n [not meant as commit message]
| > =====================================================================
| >
| > Background:
| > -----------
| > [RFC 3448, 4.6] specifies the scheduling of packet transmissions with regard to
| > scheduling granularity. The calculated sending time is called `nominal' sending
| > time t_nom. The initial sending time is called t_0. The first sending time is called
| > t_1 = t0 + t_ipi. From then on, successive sending times are calculated as
| >
| > t_nom = t_(i+1) = t_i + t_ipi
| >
| > This is illustrated by the ASCII-Art below:
| >
| >
| > |<-------------------- t_ipi ------------------->|
| >
| > +------------------------------------------------+-----------------------
| > | | |
| > | | |
| > +------------------------------------------------+-----------------------
| > | ^ |<------- t_delta ------>|
| > | | | |
| > t_i t_now t_nom - t_delta t_nom = t_(i+1)
| >
| > |<----- t_ipi - (t_now - t_i) --->|
| >
| >
| > Due to scheduling granularity, t_nom is not necessarily exactly the same as the
| > actual sending time, hence [RFC 3448, 4.6] introduced the following test:
| >
| > t_delta = min(t_ipi/2, t_gran/2); /* t_gran: scheduling granularity */
| >
| > if (t_now > t_nom - t_delta)
| > // send the packet now
| > else
| > // send the packet in t_ipi - (t_now - t_i) microseconds
| >
| > In the above illustration, t_now was too early for sending (`else' case).
| >
| >
| > Current state:
| > --------------
| > Currently the code determines nominal send times using the following pseudo-code:
| > delay_in_msec = -1/1000 * (t_now - t_nom - t_delta)
| > = 1/1000 * (t_nom + t_delta - t_now)
| >
| > if (delay_in_msec > 0) /* equivalent to t_now < t_nom + t_delta */
| > return delay_in_msec; /* i.e. reschedule in (t_nom -(t_now - t_delta))/1000 */
| > else /* whenever t_now >= t_nom + t_delta */
| > /* send packet now */
| >
| > This means that currently
| >
| > * the packet is sent now whenever t_now >= t_nom + t_delta
| > instead of t_now > t_nom - t_delta
| > ==> this incurs a performance degradation, since 2*t_delta is spent unnecessarily
| >
| > * the delay causes the packet to be rescheduled at the following point of time:
| > t_now + (t_nom + t_delta - t_now) = t_nom + t_delta
| > Hence again t_delta time units are not used.
| >
| > Solution:
| > ---------
| > The condition `if (t_now > t_nom - t_delta)' from [RFC 3448, 4.6] is equivalent with
| > the condition `if (t_nom - t_now < t_delta)' and the delay simplifies (cf. above figure):
| > t_ipi - (t_now - t_i) = t_i + t_ipi - t_now
| > = t_nom - t_now
| > Thus we can twice use `t_nom - t_now', which is what the patch does.
| >
| > ---
| > net/dccp/ccids/ccid3.c | 18 +++++++++++++-----
| > 1 file changed, 13 insertions(+), 5 deletions(-)
| >
| > --- a/net/dccp/ccids/ccid3.c
| > +++ b/net/dccp/ccids/ccid3.c
| > @@ -303,11 +303,19 @@ static int ccid3_hc_tx_send_packet(struc
| > break;
| > case TFRC_SSTATE_NO_FBACK:
| > case TFRC_SSTATE_FBACK:
| > - delay = (timeval_delta(&now, &hctx->ccid3hctx_t_nom) -
| > - hctx->ccid3hctx_delta);
| > - delay /= -1000;
| > - /* divide by -1000 is to convert to ms and get sign right */
| > - rc = delay > 0 ? delay : 0;
| > + delay = timeval_delta(&hctx->ccid3hctx_t_nom, &now);
| > + /*
| > + * Scheduling of packet transmissions [RFC 3448, 4.6]
| > + *
| > + * if (t_now > t_nom - delta)
| > + * // send the packet now
| > + * else
| > + * // send the packet in (t_nom - t_now) milliseconds.
| > + */
| > + if (delay < hctx->ccid3hctx_delta)
| > + rc = 0;
| > + else
| > + rc = delay/1000L;
| > break;
| > case TFRC_SSTATE_TERM:
| > DCCP_BUG("Illegal %s state TERM, sk=%p", dccp_role(sk), sk);
| > -
| > 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
| >
|
|
-
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
