From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: Re: fq_codel: fix CoDel drop scheduling
To: Bjorn Ketelaars <bket@openbsd.org>
Cc: tech@openbsd.org
Date: Thu, 13 Aug 2026 09:24:32 +0200

Hello Bjorn,

On Thu, Aug 13, 2026 at 03:29:48AM +0200, Bjorn Ketelaars wrote:
> On Wed 29/07/2026 08:23, Bjorn Ketelaars wrote:
> > Hi,
> >
> > The diff below fixes a few details in fq_codel's CoDel dequeue path.
> >
> > The interval table starts at interval / sqrt(1), while cd->drops is
> > one-based when control_law() is called. Use drops - 1 as the table index
> > and assert that the counter is non-zero.
> >
> > When checking the non-starvation backlog during dequeue, exclude the
> > candidate packet. The packet is still physically queued until
> > deq_commit(), but the CoDel decision is about the backlog remaining
> > after this packet is dequeued.
> >
> > Finally, enter dropping when now is equal to cd->start. cd->start marks
> > the time at which the observation interval has elapsed.
> >
> > Comments / ok?

    thanks for detailed clarification. I'm not familiar with fq_codel so
    I took my best to at least read the code around your diff.

>
> Ping, diff enclosed again for your convenience:
>
>
> diff --git sys/net/fq_codel.c sys/net/fq_codel.c
> index eb49c18aa5c..4179ee5bacd 100644
> --- sys/net/fq_codel.c
> +++ sys/net/fq_codel.c
> @@ -334,7 +334,8 @@ control_law(struct codel *cd, struct codel_params *cp, int64_t rts)
>  {
>  	unsigned int idx;
>
> -	idx = min(cd->drops, nitems(codel_intervals) - 1);
> +	KASSERT(cd->drops > 0);
> +	idx = min(cd->drops - 1, nitems(codel_intervals) - 1);
>  	cd->next = rts + cp->intervals[idx];
>  }

    the part above drags my attention. there are two places where control_law()
    is being called from:

429         while (!done) {
430                 m = codel_next_packet(cd, cp, now, &drop);
431                 state = codel_state_change(cd, now, m, drop, state);
432
433                 switch (state) {
434                 case FIRSTDROP:
...
450                         delta = cd->drops - cd->ldrops;
451                         if (delta > 1 && (now < cd->next ||
452                             now - cd->next < cp->grace))
453                                 cd->drops = delta;
454                         else
455                                 cd->drops = 1;
456                         control_law(cd, cp, now);
457                         cd->ldrops = cd->drops;
458
459                         /* fetches the next packet and goes to ACCEPTING */
460                         break;
...
471                 case CONTROL:
472                         if (drop) {
473                                 control_law(cd, cp, cd->next);
474                                 continue;
475                         }
476                         /* FALLTHROUGH */

    the branch for FIRSTDROP is clear as it does make sure
    cd->drops is at least 1. For other branch CONTROL I was trying
    to decode a state transitions which happen at lines 430+431 but
    it looks like the code in CONTROL branch is actually unreachable.
    I have not test my suspicion by trying to configure the pf with queues.
    I could miss something there.


    the diff reads good to me. anything else can be part of follow up
    commit when needed.

feel free to commit with OK sashan@

thanks and
regards
sashan