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

[Jakub Kicinski <kuba@kernel.org>]

On Sun, 27 Jul 2025 23:56:32 +0000 William Liu wrote:
> Special care is taken for fq_codel_dequeue to account for the
> qdisc_tree_reduce_backlog call in its dequeue handler. The
> cstats reset is moved from the end to the beginning of
> fq_codel_dequeue, so the change handler can use cstats for
> proper backlog reduction accounting purposes. The drop_len and
> drop_count fields are not used elsewhere so this reordering in
> fq_codel_dequeue is ok.

Using local variables like we do in other qdiscs will not work?
I think your change will break drop accounting during normal dequeue?

> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 638948be4c50..a24094a638dc 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -1038,10 +1038,15 @@ static inline struct sk_buff *qdisc_dequeue_internal(struct Qdisc *sch, bool dir
>  	skb = __skb_dequeue(&sch->gso_skb);
>  	if (skb) {
>  		sch->q.qlen--;
> +		qdisc_qstats_backlog_dec(sch, skb);
> +		return skb;
> +	}
> +	if (direct) {
> +		skb = __qdisc_dequeue_head(&sch->q);
> +		if (skb)
> +			qdisc_qstats_backlog_dec(sch, skb);
>  		return skb;
>  	}
> -	if (direct)
> -		return __qdisc_dequeue_head(&sch->q);
>  	else

sorry for a late nit, it wasn't very clear from the diff but
we end up with

	if (direct) {
		...
	}
	else
		return ..;

Please reformat:

	if (direct) {
		...
	} else {
		...
	}

>  		return sch->dequeue(sch);
>  }

> diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> index 902ff5470607..986e71e3362c 100644
> --- a/net/sched/sch_fq.c
> +++ b/net/sched/sch_fq.c
> @@ -1014,10 +1014,10 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
>  		     struct netlink_ext_ack *extack)
>  {
>  	struct fq_sched_data *q = qdisc_priv(sch);
> +	unsigned int prev_qlen, prev_backlog;
>  	struct nlattr *tb[TCA_FQ_MAX + 1];
> -	int err, drop_count = 0;
> -	unsigned drop_len = 0;
>  	u32 fq_log;
> +	int err;
>  
>  	err = nla_parse_nested_deprecated(tb, TCA_FQ_MAX, opt, fq_policy,
>  					  NULL);
> @@ -1135,16 +1135,16 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
>  		err = fq_resize(sch, fq_log);
>  		sch_tree_lock(sch);
>  	}
> +
> +	prev_qlen = sch->q.qlen;
> +	prev_backlog = sch->qstats.backlog;
>  	while (sch->q.qlen > sch->limit) {
>  		struct sk_buff *skb = qdisc_dequeue_internal(sch, false);
>  
> -		if (!skb)
> -			break;

The break conditions is removed to align the code across the qdiscs?

> -		drop_len += qdisc_pkt_len(skb);
>  		rtnl_kfree_skbs(skb, skb);
> -		drop_count++;
>  	}
> -	qdisc_tree_reduce_backlog(sch, drop_count, drop_len);
> +	qdisc_tree_reduce_backlog(sch, prev_qlen - sch->q.qlen,
> +				  prev_backlog - sch->qstats.backlog);

There is no real change in the math here, right?
Again, you're just changing this to align across the qdiscs?
-- 
pw-bot: cr