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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 3 Mar 2011 09:19:40 +0100
From:	Fabio Checconi <fchecconi@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Stephen Hemminger <shemminger@...tta.com>,
	David Miller <davem@...emloft.net>,
	Luigi Rizzo <rizzo@....unipi.it>, netdev@...r.kernel.org,
	Paolo Valente <paolo.valente@...more.it>
Subject: Re: [PATCH] sched: QFQ - quick fair queue scheduler (v2)

Hi,

> From: Eric Dumazet <eric.dumazet@...il.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...
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html