Re: [PATCH net v4 1/2] net/sched: Fix backlog accounting in qdisc_dequeue_internal

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 12 Aug 2025 02:10:02 +0000 William Liu wrote:
> > AFAICT only if the backlog adjustment is using the prev_qlen,
> > prev_backlog approach, which snapshots the backlog. In that case,
> > yes, the "internal drops" will mess up the count.  
> 
> Yep, that's why I added the dropped_qlen and dropped_backlog
> variables, though that is not a very pretty solution.
> 
> But even looking at the method you suggested (copy pasting for
> reference):
> 
> 	pkts = 0;
> 	bytes = 0;
>  	while (sch->q.qlen > sch->limit ||
>  	       q->memory_usage > q->memory_limit) {
>  		struct sk_buff *skb = qdisc_dequeue_internal(sch, false); 
> 		pkts++;
> 		bytes += qdisc_pkt_len(skb);
>  		rtnl_kfree_skbs(skb, skb);
>  	}
> 	qdisc_tree_reduce_backlog(sch, pkts, bytes);
> 
> qdisc_dequeue_internal can trigger fq_codel_dequeue, which can
> trigger qdisc_tree_reduce_backlog before returning (the only qdisc
> out of these that does so in its dequeue handler). 
> 
> Let's say the limit only goes down by one, and packet A is at the
> front of the queue. qdisc_dequeue_internal takes the dequeue path,
> and fq_codel_dequeue triggers a qdisc_tree_reduce_backlog from that
> packet before returning the skb. Would this final
> qdisc_tree_reduce_backlog after the limit drop not double count? 

The packets that got counted in qdisc_tree_reduce_backlog() inside
->dequeue are freed immediately via

  drop_func()
    kfree_skb_reason(skb, SKB_DROP_REASON_QDISC_CONGESTED);

in the scenario you're describing ->dequeue should return NULL.
If that's possible, then we have another bug here :$

Normally backlogs get adjusted as the packet travels down the hierarchy
thru the parent chain. ->dequeue is part of this normal path so skbs it
returns are still in the parent's backlogs. qdisc_tree_reduce_backlog()
is only called when we need to do something to an skb outside of the
normal ->enqueue/->dequeue flow that iterates the hierarchy.