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

Eric Dumazet
2011-03-03T08:37:25Z

Le jeudi 03 mars 2011 à 09:19 +0100, Fabio Checconi a écrit :
> 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.
>

Strange, I used the default qdisc (pfifo) created in qfq classes

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

I am going to test an updated version, thanks for all these hints !