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

[Eric Dumazet <eric.dumazet@gmail.com>]

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 !