Re: [PATCH] Fixed NetEm's reorder option

Re: [PATCH] Fixed NetEm's reorder option

[Rafael Almeida]

I noticed no one said anything about my patch below, did I do
something terrible wrong? Should I just be more patient?

On Thu, Jun 5, 2008 at 1:34 AM, Rafael C. de Almeida
<almeidaraf@gmail.com> wrote:
> The reorder option wasn't working correctly. It would only kick in if
> the gap option was set. In that case it would only lower the
> probability of reordering packets because of how the if conditional
> was stated.
>
> This patch separetes the algorithm for reordering with gap from the
> rest, rendering the code more readable and correct.
>
> Signed-off-by: Rafael C. de Almeida <rafaelc@dcc.ufmg.br>
> ---
>  net/sched/sch_netem.c |   45 ++++++++++++++++++++++++++++++++-------------
>  1 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index c9c649b..477ee09 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -206,27 +206,46 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>        }
>
>        cb = (struct netem_skb_cb *)skb->cb;
> -       if (q->gap == 0                 /* not doing reordering */
> -           || q->counter < q->gap      /* inside last reordering gap */
> -           || q->reorder < get_crandom(&q->reorder_cor)) {
> +       if (q->gap) {
> +               /* The comparison of reorder is here to keep compatibility with
> +                * the old behaviour when one had reorder and gap set, maybe it
> +                * could be removed.
> +                */
> +               if (q->counter < q->gap
> +                   || q->reorder < get_crandom(&q->reorder_cor)) {
> +                       psched_time_t now;
> +                       psched_tdiff_t delay;
> +
> +                       delay = tabledist(q->latency, q->jitter,
> +                                         &q->delay_cor, q->delay_dist);
> +
> +                       now = psched_get_time();
> +                       cb->time_to_send = now + delay;
> +                       ++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.
> +                        */
> +                       cb->time_to_send = psched_get_time();
> +                       q->counter = 0;
> +                       ret = q->qdisc->ops->requeue(skb, q->qdisc);
> +               }
> +       } else {
>                psched_time_t now;
>                psched_tdiff_t delay;
>
> -               delay = tabledist(q->latency, q->jitter,
> -                                 &q->delay_cor, q->delay_dist);
> +               if (!q->reorder || q->reorder < get_crandom(&q->reorder_cor))
> +                       delay = tabledist(q->latency, q->jitter,
> +                                         &q->delay_cor, q->delay_dist);
> +               else
> +                       delay = 0;
>
>                now = psched_get_time();
>                cb->time_to_send = now + delay;
>                ++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.
> -                */
> -               cb->time_to_send = psched_get_time();
> -               q->counter = 0;
> -               ret = q->qdisc->ops->requeue(skb, q->qdisc);
>        }
>
>        if (likely(ret == NET_XMIT_SUCCESS)) {
> --
> 1.5.5.GIT
>
>

Re: [PATCH] Fixed NetEm's reorder option

[Stephen Hemminger]

On Wed, 11 Jun 2008 17:05:10 -0300
"Rafael Almeida" <almeidaraf@gmail.com> wrote:

> I noticed no one said anything about my patch below, did I do
> something terrible wrong? Should I just be more patient?
> 
> On Thu, Jun 5, 2008 at 1:34 AM, Rafael C. de Almeida
> <almeidaraf@gmail.com> wrote:
> > The reorder option wasn't working correctly. It would only kick in if
> > the gap option was set. In that case it would only lower the
> > probability of reordering packets because of how the if conditional
> > was stated.
> >
> > This patch separetes the algorithm for reordering with gap from the
> > rest, rendering the code more readable and correct.
> >
> > Signed-off-by: Rafael C. de Almeida <rafaelc@dcc.ufmg.br>

Haven't had time to look at this in detail, and I worry about the impact
of changing assumptions people already have in their scripts.