[LARTC] [HTB] htb_dequeue_tree assertion (kernel 2.4.21-ac4)

[devik]

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.
  Send mail to mime@docserver.cac.washington.edu for more info.

--8323328-446180073-1058945995=:18108
Content-Type: TEXT/PLAIN; charset=US-ASCII

Hi,

try attached fix please (it duplicates last one too so that
you might get a reject).

-------------------------------
    Martin Devera aka devik
Linux kernel QoS/HTB maintainer
  http://luxik.cdi.cz/~devik/

On Sun, 20 Jul 2003, Wilfried Weissmann wrote:

> devik wrote:
> >>>If you read comment above htb_dequeue_tree, it should be called
> >>>only when it is sure that there are packets inside of the level/prio.
> >>>It is known by other HTB mechanism (per-level activity lists).
> >>>
> >>>Thus the bugtrap is to catch case where class was inserted
> >>>into activity list because it had packets in its sub-qdisc
> >>>but when we actually decide to dequeue - it has no packet.
> >>>It is weird - can qdisc lose packets even when dequeue was
> >>>not called ??
> >>
> >>If you change the depth of the leave queue then it is possible to drop
> >>packets or if you completely exchange the queue. Which would also
> >>explain why the assertion only occurs when the configuration is altered.
> >
> >
> > Well, I agree that there is something wrong. Now it is neccessary to
> > find scenario where it does happen so that it is fixed in right way.
> > I have not much time these days to test these cases but your informations
> > would lead to following hypothesis:
> >
> > Classe's child qdisc is replaced while old one still has nonzero queue.
> > New empty qdisc is grafted under class instead. What about attached
> > patch (it is against my latest version so you can see offset warnings) ?
>
> This would not work if there are several intermediates HTB queues from
> the device to the leave queue. In this case every queue from the queue
> that was changed to the root has to be notified about the change. (The
> setup we want to use involves such a configuration.) Maybe it is better
> to just deactivate a class when a dequeue from its leave failes due to a
> zero queue length. If you are concerned about performance then an audit
> process could be implemented. For example to check one leave queue every
> 64 packets +/- initial random offset to create some entropy similar to
> the maximum mount count in the ext2 filesystem. Maybe there are better
> ways to do this. I am not so familiar with the code.
>
> I will make some tests with the patch tomorrow. If my theory is true
> then it should still help a lot.
>
> bye,
> wilfried
>
> >
> > devik
> >
> >
> > ------------------------------------------------------------------------
> >
> > --- sch_htb.c	2003/07/05 10:37:11	1.21
> > +++ sch_htb.c	2003/07/20 07:24:59
> > @@ -1286,6 +1286,10 @@ static int htb_graft(struct Qdisc *sch,
> >  					return -ENOBUFS;
> >  		sch_tree_lock(sch);
> >  		if ((*old = xchg(&cl->un.leaf.q, new)) != NULL) {
> > +			/* TODO: test it */
> > +			if (cl->prio_activity)
> > +				htb_deactivate ((struct htb_sched*)sch->data,cl);
> > +
> >  			/* TODO: is it correct ? Why CBQ doesn't do it ? */
> >  			sch->q.qlen -= (*old)->q.qlen;
> >  			qdisc_reset(*old);
>
>
>
>
>
>

--8323328-446180073-1058945995=:18108
Content-Type: TEXT/PLAIN; charset=US-ASCII; name="htb_deq_tree_fix1.diff"
Content-Transfer-Encoding: BASE64
Content-ID: <Pine.LNX.4.33.0307230939550.18108@devix>
Content-Description: 
Content-Disposition: attachment; filename="htb_deq_tree_fix1.diff"

LS0tIHNjaF9odGIuYwkyMDAzLzA3LzA1IDEwOjM3OjExCTEuMjENCisrKyBz
Y2hfaHRiLmMJMjAwMy8wNy8yMyAwNzozNzo1Mg0KQEAgLTk0NywxNSArOTQ3
LDI0IEBAIHN0YXRpYyBzdHJ1Y3Qgc2tfYnVmZiAqDQogaHRiX2RlcXVldWVf
dHJlZShzdHJ1Y3QgaHRiX3NjaGVkICpxLGludCBwcmlvLGludCBsZXZlbCkN
CiB7DQogCXN0cnVjdCBza19idWZmICpza2IgPSBOVUxMOw0KLQkvL3N0cnVj
dCBodGJfc2NoZWQgKnEgPSAoc3RydWN0IGh0Yl9zY2hlZCAqKXNjaC0+ZGF0
YTsNCiAJc3RydWN0IGh0Yl9jbGFzcyAqY2wsKnN0YXJ0Ow0KIAkvKiBsb29r
IGluaXRpYWwgY2xhc3MgdXAgaW4gdGhlIHJvdyAqLw0KIAlzdGFydCA9IGNs
ID0gaHRiX2xvb2t1cF9sZWFmIChxLT5yb3dbbGV2ZWxdK3ByaW8scHJpbyxx
LT5wdHJbbGV2ZWxdK3ByaW8pOw0KIAkNCiAJZG8gew0KLQkJQlVHX1RSQVAo
Y2wgJiYgY2wtPnVuLmxlYWYucS0+cS5xbGVuKTsgaWYgKCFjbCkgcmV0dXJu
IE5VTEw7DQorCQlCVUdfVFJBUChjbCk7IA0KKwkJaWYgKCFjbCkgcmV0dXJu
IE5VTEw7DQogCQlIVEJfREJHKDQsMSwiaHRiX2RlcV90ciBwcmlvPSVkIGxl
dj0lZCBjbD0lWCBkZWZpYz0lZFxuIiwNCiAJCQkJcHJpbyxsZXZlbCxjbC0+
Y2xhc3NpZCxjbC0+dW4ubGVhZi5kZWZpY2l0W2xldmVsXSk7DQorDQorCQkv
KiBjbGFzcyBjYW4gYmUgZW1wdHkgLSBpdCBpcyB1bmxpa2VseSBidXQgY2Fu
IGJlIHRydWUgaWYgbGVhZg0KKwkJICAgcWRpc2MgZHJvcHMgcGFja2V0cyBp
biBlbnF1ZXVlIHJvdXRpbmUgb3IgaWYgc29tZW9uZSB1c2VkDQorCQkgICBn
cmFmdCBvcGVyYXRpb24gb24gdGhlIGxlYWYgc2luY2UgbGFzdCBkZXF1ZXVl
OyANCisJCSAgIHNpbXBseSBkZWFjdGl2YXRlIGFuZCBza2lwIHN1Y2ggY2xh
c3MgKi8NCisJCWlmICh1bmxpa2VseShjbC0+dW4ubGVhZi5xLT5xLnFsZW4g
PT0gMCkpIHsNCisJCQlodGJfZGVhY3RpdmF0ZShxLGNsKTsNCisJCQlnb3Rv
IG5ld19sb29rdXA7DQorCQl9DQogCQ0KIAkJaWYgKGxpa2VseSgoc2tiID0g
Y2wtPnVuLmxlYWYucS0+ZGVxdWV1ZShjbC0+dW4ubGVhZi5xKSkgIT0gTlVM
TCkpIA0KIAkJCWJyZWFrOw0KQEAgLTk2NSw2ICs5NzQsNyBAQCBodGJfZGVx
dWV1ZV90cmVlKHN0cnVjdCBodGJfc2NoZWQgKnEsaW50DQogCQl9DQogCQlx
LT5ud2NfaGl0Kys7DQogCQlodGJfbmV4dF9yYl9ub2RlKChsZXZlbD9jbC0+
cGFyZW50LT51bi5pbm5lci5wdHI6cS0+cHRyWzBdKStwcmlvKTsNCituZXdf
bG9va3VwOg0KIAkJY2wgPSBodGJfbG9va3VwX2xlYWYgKHEtPnJvd1tsZXZl
bF0rcHJpbyxwcmlvLHEtPnB0cltsZXZlbF0rcHJpbyk7DQogCX0gd2hpbGUg
KGNsICE9IHN0YXJ0KTsNCiANCkBAIC0xMjg2LDYgKzEyOTYsMTAgQEAgc3Rh
dGljIGludCBodGJfZ3JhZnQoc3RydWN0IFFkaXNjICpzY2gsIA0KIAkJCQkJ
cmV0dXJuIC1FTk9CVUZTOw0KIAkJc2NoX3RyZWVfbG9jayhzY2gpOw0KIAkJ
aWYgKCgqb2xkID0geGNoZygmY2wtPnVuLmxlYWYucSwgbmV3KSkgIT0gTlVM
TCkgew0KKwkJCS8qIFRPRE86IHRlc3QgaXQgKi8NCisJCQlpZiAoY2wtPnBy
aW9fYWN0aXZpdHkpDQorCQkJCWh0Yl9kZWFjdGl2YXRlICgoc3RydWN0IGh0
Yl9zY2hlZCopc2NoLT5kYXRhLGNsKTsNCisNCiAJCQkvKiBUT0RPOiBpcyBp
dCBjb3JyZWN0ID8gV2h5IENCUSBkb2Vzbid0IGRvIGl0ID8gKi8NCiAJCQlz
Y2gtPnEucWxlbiAtPSAoKm9sZCktPnEucWxlbjsJDQogCQkJcWRpc2NfcmVz
ZXQoKm9sZCk7DQo=
--8323328-446180073-1058945995=:18108--