Re: [PATCH] netem: fix logic bug in reorder conditional

[Julio Kriger <juliokriger@gmail.com>]

Hi!

1) Supouse I don't want reordering, so in 'tc' I don't add reorder
option. Will this code work? The previous (on this subject) patch has
a the following:

+       /* for compatiablity with earlier versions.
+        * if gap is set, need to assume 100% probablity
+        */
+       q->reorder = ~0;
+

Should it be set to 100% or 101%? 
This "q->reorder = ~0;" mean 100%?

2) If I set latency = 50ms and a jitter = 300ms, tabledist can give me
a negative number. This value is addes to cb->time_to_send, so it
could change it to a negative value. Should we only accept positives
number before add it to cb->time_to_send? or will
q->qdisc->enqueue(skb, q->qdisc) put the package on the queue in a
special way so it will be handled "before" other packages alrealy on
the queue but with gretaer time_to_send?

Regards,
Julio

On 5/23/05, Stephen Hemminger <shemminger@osdl.org> wrote:
> Thanks, Julio you spotted the problem with the logic.  This should fix it:
> 
> Index: netem-2.6.12-rc4/net/sched/sch_netem.c
> ===================================================================
> --- netem-2.6.12-rc4.orig/net/sched/sch_netem.c
> +++ netem-2.6.12-rc4/net/sched/sch_netem.c
> @@ -181,13 +181,9 @@ static int netem_enqueue(struct sk_buff
>                 q->duplicate = dupsave;
>         }
> 
> -       /*
> -        * Do re-ordering by putting one out of N packets at the front
> -        * of the queue.
> -        * gap == 0 is special case for no-reordering.
> -        */
> -       if (q->gap == 0 && q->counter < q->gap &&
> -           q->reorder < get_crandom(&q->reorder_cor)) {
> +       if (q->gap == 0                 /* not doing reordering */
> +           || q->counter < q->gap      /* inside last reordering gap */
> +           || q->reorder < get_crandom(&q->reorder_cor)) {
>                 psched_time_t now;
>                 PSCHED_GET_TIME(now);
>                 PSCHED_TADD2(now, tabledist(q->latency, q->jitter,
> @@ -196,6 +192,10 @@ static int netem_enqueue(struct sk_buff
>                 ++q->counter;
>                 ret = q->qdisc->enqueue(skb, q->qdisc);
>         } else {
> +               /*
> +                * Do re-ordering by putting one out of N packets at the front
> +                * of the queue.
> +                */
>                 PSCHED_GET_TIME(cb->time_to_send);
>                 q->counter = 0;
>                 ret = q->qdisc->ops->requeue(skb, q->qdisc);
> 


-- 
----------------------------
Julio Kriger
mailto:juliokriger@gmail.com