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

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

Le jeudi 03 mars 2011 à 08:48 -0800, Stephen Hemminger a écrit :
> This is an implementation of the Quick Fair Queue scheduler developed
> by Fabio Checconi. The same algorithm is already implemented in ipfw
> in FreeBSD. Fabio had an earlier version developed on Linux, I just
> cleaned it up.  Thanks to Eric Dumazet for doing the testing and
> finding bugs.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> ---
> v3 - bug fixes found by Eric
> 
>  include/linux/pkt_sched.h |   15 
>  net/sched/Kconfig         |   11 
>  net/sched/Makefile        |    1 
>  net/sched/sch_qfq.c       | 1125 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1152 insertions(+)
> 


Hmm... please compile before sending your patches ;)

You used sched_priv(sch) calls instead of qdisc_priv(sch)

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

	q = qdisc_priv(sch);

> +
> +	if (cl->inv_w) {
> +		q->wsum -= ONE_FP / cl->inv_w;
> +		cl->inv_w = 0;
> +	}
> +
> +	gen_kill_estimator(&cl->bstats, &cl->rate_est);
> +	qdisc_destroy(cl->qdisc);
> +	kfree(cl);
> +}

> +
> +static void qfq_qlen_notify(struct Qdisc *sch, unsigned long arg)
> +{
> +	struct qfq_sched *q = sched_priv(sch);

	q = qdisc_priv(sch);

> +	struct qfq_class *cl = (struct qfq_class *)arg;
> +
> +	if (cl->qdisc->q.qlen == 0)
> +		qfq_deactivate_class(q, cl, NULL);
> +}
> +



> +static void qfq_reset_qdisc(struct Qdisc *sch)
> +{
> +	struct qfq_sched *q = qdisc_priv(sch);
> +	struct qfq_group *grp;
> +	struct qfq_class *cl, **pp;
> +	struct hlist_node *n;
> +	unsigned int i, j;
> +
> +	for (i = 0; i <= QFQ_MAX_INDEX; i++) {
> +		grp = &q->groups[i];
> +		for (j = 0; j < QFQ_MAX_SLOTS; j++) {
> +			for (pp = &grp->slots[j]; *pp; pp = &(*pp)->next) {
> +				cl = *pp;
> +				if (cl->qdisc->q.qlen)
> +					qfq_deactivate_class(q, cl, pp);


Here there is the problem of *pp = cl->next (possibly NULL)

		maybe use

	for (pp = &grp->slots[j]; (cl = *pp) != NULL;) {
		if (cl->qdisc->q.len)
			qfq_deactivate_class(...);
		else
			pp = &cl->next;
	}


> +			}
> +		}
> +	}
> +
> +	for (i = 0; i < q->clhash.hashsize; i++) {
> +		hlist_for_each_entry(cl, n, &q->clhash.hash[i], common.hnode)
> +			qdisc_reset(cl->qdisc);
> +	}
> +	sch->q.qlen = 0;
> +}
> +