On 04/10/15 11:50, Sebastian Moeller wrote: > Hi Kevin, > > > On Oct 3, 2015, at 21:42 , Kevin Darbyshire-Bryant wrote: > >> Hello All, >> >> On 18th Aug (before I'd subscribed to the list), Dave posted a message >> about reviewing cake >> https://lists.bufferbloat.net/pipermail/cake/2015-August/000364.html >> >> In it he mentioned a change to cake_drop: >> >> " >> >> I would like cake_drop to be evaluated with some less exaustive >> search. It also looks wrong >> >> for(j=0; j < CAKE_MAX_CLASSES; j++) { >> fqcd = &q->classes[j]; >> >> CAKE_MAX_CLASSES should actually be q->class_cnt here; I think. I am >> unsure if other references to it are correct in face of changes, on >> cleanup" >> >> >> After making the change and nearly submitting a pull request I've >> thought about this some more and to be blunt it makes me nervous. My >> concern is what happens if cake is changed from say a diffserv8 config >> to a single class(bin) config and for whatever reason 'cake_drop' is >> called. with diffserv8 the fat flow is likely to be in a class(bin) >> higher than the first....cake_drop would now only check the first class >> for the fat flow and possibly not find anything to drop.... I fear what >> happens next! > > I thought the following takes care of that (called from cake_reconfigure() ): > > /* Discard leftover packets from a class no longer in use. */ > static void cake_clear_class(struct Qdisc *sch, u16 class) > { > struct cake_sched_data *q = qdisc_priv(sch); > struct cake_fqcd_sched_data *fqcd = &q->classes[class]; > > q->cur_class = class; > for(q->cur_flow = 0; q->cur_flow < fqcd->flows_cnt; q->cur_flow++) > while(custom_dequeue(NULL, sch)) > ; > } > > I probably do no understand things fully enough, but to me this seems to take care of your concern. Especially since cake_reconfigure will only touch classes from q->class_cnt up to CAKE_MAX_CLASSES. Since I am not very fluent in C I might have missed things… That means that exchanging CAKE_MAX_CLASSES with q->class_cnt in cake_drop() should be safe. The question left is then how costly is it to search empty classes? I guess, cake_drop is the slow path and hence could afford to search them all, then again cake_drop probably tests called most under heavy traffic conditions, so even the slow path performance might be noticeable to the user… Good spot Sebastian! I agree that looking through the code would indicate that a reconfigure from say 'diffserv8' to 'besteffort' would actually cause cake to drop all the packets in the now redundant classes, therefore my worry is solved. I've re-submitted the pull request. I may even merge it as it has been running on my router for a couple of days without actually blowing up :-)