Re: [PATCH] sched: QFQ - quick fair queue scheduler (v2)

Fabio Checconi
2011-03-03T08:17:58Z

Hi,

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, Mar 02, 2011 06:31:27PM +0100
>
> Le mercredi 02 mars 2011 à 17:18 +0100, Eric Dumazet a écrit :
> > Le mercredi 02 mars 2011 à 08:11 -0800, Stephen Hemminger a écrit :
> >
> > > I put the iproute2 code into the repository in the experimental branch.
> > >
> >
> > Thanks
> >
> > It seems as soon as packets are dropped, qdisc is frozen (no more
> > packets dequeued)

I've been able to reproduce this problem using netem, and it seems to
be due to this check:

> + /* If the new skb is not the head of queue, then done here. */
> + if (skb != qdisc_peek_head(cl->qdisc))
> + return err;

changing it to:

if (cl->qdisc->q.qlen != 1)
return err;

seems to make things work. I think this is because qdisc_peek_head()
looks at the wrong list, as the skb is not queued directly in q, but
ends up in the child qdisc attached to cl->qdisc.

> >
> > Hmm...
> >
>
> It seems class deletes are buggy.
>
> After one "tc class del dev $ETH classid 11:1 ..."
>
> a "tc -s -d qdisc show dev $ETH" triggers an Oops
>

This seems to be due to:

> +static void qfq_destroy_class(struct Qdisc *sch, struct qfq_class *cl)
> +{
> + struct qfq_sched *q = (struct qfq_sched *)sch;

it should be:

struct qfq_sched *q = sched_priv(sch);

The same bug you identified in qfq_reset_qdisc() is present also in
qfq_drop(), both loops need to be corrected...

It should also be noted that this scheduler (like HFSC, IIRC) depends
on the child qdisc not to reorder packets for the guarantees to be met,
as the timestamps need to be in sync with the length of the packet at the
head of the queue. If this can't be guaranteed, to preserve the formal
correctness it should be changed to always use the maximum packet size
to calculate the timestamps.

@Stephen: not that I'm proud of that, but all the bugs found so far are mine...