Re: [PATCH net-next v3 1/3] net/sched: drr, ets, sfq: skip empty rounds in the deficit-refill dequeue loop

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The bulk-refill scan in drr_dequeue()
  (net/sched/sch_drr.c:422-431) and ets_qdisc_dequeue()…
- [Low] The commit "net/sched: drr, ets, sfq: skip empty rounds in the
  deficit-refill dequeue loop" fixes a dequeue stall: one dequeue spins…

--

net/sched: drr, ets, sfq: skip empty rounds in the deficit-refill dequeue loop

This patch changes the DRR, ETS and SFQ dequeue loops. After a full round
in which no class could dequeue, they compute how many rounds remain and
credit every active class that many quanta at once. It also takes the tree
lock when a DRR or ETS class quantum is changed.

Link: https://lore.kernel.org/netdev/3ab59725-e34c-4243-bc72-d93f837bcad1@redhat.com/
Link: https://lore.kernel.org/netdev/CA+NMeC_kwd5yihR3SSqb+Z8wX+NuobntbvpKtAyOwjZ1Xa4mVg@mail.gmail.com/

> Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260819143213.57401-1-jhs@mojatatu.com

[Severity: Low]
Should this have a Fixes: tag? The patch fixes a dequeue stall: one dequeue
spins up to N * 4096 times under the root lock with BH disabled. It has
Reported-by and Closes trailers, but no Fixes:.

The earlier commit 8f735d64382d ("net/sched: bound qdisc_pkt_len to
prevent qdisc soft lockup") fixed the unbounded form of the same refill
loop, and it carried Fixes: 1da177e4c3f4. Without a Fixes: tag here,
stable has nothing to target for the remaining bounded but long form of
the loop.

> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> index 8621d057edd9f..bff7b307e98eb 100644
> --- a/net/sched/sch_drr.c
> +++ b/net/sched/sch_drr.c

[ ... ]

> @@ -402,6 +410,51 @@ static struct sk_buff *drr_dequeue(struct Qdisc *sch)
>  			return skb;
>  		}
>  
> +		/* No class could dequeue in the last full round. Add the
> +		 * quanta for the rounds still needed in one pass.
> +		 */
> +		if (!scanned && cl == first) {
> +			unsigned int rounds = UINT_MAX;
> +			struct drr_class *iter, *tmp;
> +			bool skip = true;
> +
> +			scanned = true;
> +			list_for_each_entry_safe(iter, tmp, &q->active, alist) {
> +				unsigned int pkt_len;
> +				struct sk_buff *peek;
> +
> +				/* A child that emptied itself while peeking
> +				 * is no longer in the list and needs no rounds.
> +				 */
> +				peek = iter->qdisc->ops->peek(iter->qdisc);
> +				if (!peek)
> +					continue;
> +				pkt_len = qdisc_pkt_len(peek);
> +				if (pkt_len <= iter->deficit) {
> +					skip = false;
> +					break;
> +				}
> +				rounds = min(rounds,
> +					     DIV_ROUND_UP(pkt_len - iter->deficit,
> +							  READ_ONCE(iter->quantum)));
> +			}
> +			if (skip && rounds != UINT_MAX) {
> +				list_for_each_entry(iter, &q->active, alist)
> +					WRITE_ONCE(iter->deficit,
> +						   iter->deficit + rounds *
> +						   READ_ONCE(iter->quantum));

[Severity: Medium]
Is the comment above the peek accurate? When peek() returns NULL in
drr_dequeue() the class is skipped with continue. Nothing checks that it
has actually left q->active.

If the class is still linked, its need never limited rounds. But the
refill loop above walks all of q->active and adds rounds * quantum to
that class too.

Can a backlogged child return NULL here after it returned a packet earlier
in the same round? multiq_peek() does not cache its result, and it skips
bands whose TX queue is stopped:

net/sched/sch_multiq.c:multiq_peek() {
    ...
		if (!netif_xmit_stopped(
		    netdev_get_tx_queue(qdisc_dev(sch), curband))) {
			qdisc = q->queues[curband];
			skb = qdisc->ops->peek(qdisc);
    ...
}

Another CPU can change the TX queue state while this CPU holds the root
lock. Examples are a qdisc-bypass sender going through dev_direct_xmit(),
or a driver reset or link-down path.

multiq can be grafted under a DRR or ETS class. Such a class stays on
q->active because it still has packets.

Every class that is counted has deficit >= quantum >= 256, and pkt_len is
bounded by QDISC_PKT_LEN_MAX. So rounds <= 4095, and rounds * quantum on
its own fits in a u32.

The sum can still wrap. Take a skipped class B with quantum 1 << 20 that
already got its quantum this round, so its deficit is in [2^20, 2^21).
If a quantum 256 class needs 4095 rounds, then:

    iter->deficit + rounds * READ_ONCE(iter->quantum)

wraps B's u32 deficit. With a smaller rounds nothing wraps, but B can still
gain up to about 4 GiB of credit. It can then send thousands of
stab-inflated packets back to back and starve the other classes. Each
repeated scan after a refill can add more credit.

In the old loop, a NULL peek at the head ended the dequeue through
qdisc_warn_nonwc() and goto out. A class gained at most one quantum per
visit.

Does this conflict with the commit message statement that "Deficits and
service order are the same as with the one-quantum loop"?

One option is to abandon the bulk refill (skip = false; break) when peek()
returns NULL and !list_empty(&iter->alist). Another is to credit only the
classes that were counted. Either way, the comment above the peek would
need updating.

> +				/* A child may show a longer head now that its
> +				 * deficit is positive; scan again if it still
> +				 * cannot send.
> +				 */
> +				first = NULL;
> +				scanned = false;
> +				continue;
> +			}
> +		}

[ ... ]

> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> index 6cc902a038387..86fdf772fe2e5 100644
> --- a/net/sched/sch_ets.c
> +++ b/net/sched/sch_ets.c

[ ... ]

> @@ -493,6 +500,50 @@ static struct sk_buff *ets_qdisc_dequeue(struct Qdisc *sch)

[ ... ]

> +				peek = iter->qdisc->ops->peek(iter->qdisc);
> +				if (!peek)
> +					continue;

[ ... ]

> +			if (skip && rounds != UINT_MAX) {
> +				list_for_each_entry(iter, &q->active, alist)
> +					iter->deficit += rounds *
> +							 READ_ONCE(iter->quantum);

[Severity: Medium]
The same pattern is in ets_qdisc_dequeue(). A class whose child peek()
returns NULL is skipped without checking that it left q->active. It then
gets rounds * quantum in the refill loop, and the ETS quantum can also be
up to 1 << 20.

Would the same fix apply here?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-TIAE.v3.r0.20261109173518%40mojatatu.com