Re: [PATCH net] net/sched: re-enable queue reset on root qdisc graft

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 14 May 2026 11:12:41 +0800 Jiayuan Chen wrote:
> Commit 47e8dbb6e763 ("net/sched: do not reset queues in graft
> operations") changed dev_deactivate() in qdisc_graft() from
> reset_needed=true to false. This was the right call for graft paths
> where the new qdisc has an ->attach op (mq): the new root
> takes over per-tx-queue state via attach, and a blanket reset would
> needlessly drop packets in unrelated leaves on every graft.
> 
> For the path where the new qdisc has no ->attach (e.g. HTB, sfq
> as root, or qdisc_graft() called for deletion with new == NULL), the
> old root subtree is going to be torn down anyway: every leaf will be
> freed shortly via __qdisc_destroy(). Skipping the early reset there
> provides no benefit, but it leaves leaf qdiscs with their queues
> intact during the window between rcu_assign_pointer(dev->qdisc, new)
> and the per-leaf sfq_destroy()/timer_delete_sync(). If a leaf has a
> self-armed timer that walks the parent chain (sfq_perturbation ->
> sfq_rehash -> qdisc_tree_reduce_backlog), the timer can fire after the
> old root has been swapped out, find dev->qdisc no longer matching,
> and trigger WARN_ON_ONCE(parentid != TC_H_ROOT).

Is not resetting root really worth the extra code?
We care about mq not resetting for each child N times
more than we care about the mq itself?

If we do care - you're breaking reverse xmas tree,
and "need_skip" is quite a poor choice of a name for
readability. 

> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 6f7847c5536f..932cd1144b2b 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1097,6 +1097,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>  	struct net *net = dev_net(dev);
>  
>  	if (parent == NULL) {
> +		bool need_skip = false;
>  		unsigned int i, num_q, ingress;
>  		struct netdev_queue *dev_queue;
>  
> @@ -1123,12 +1124,15 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>  			}
>  		}
>  
> +		if (new && new->ops->attach && !ingress)
> +			need_skip = true;
> +
>  		if (dev->flags & IFF_UP)
> -			dev_deactivate(dev, false);
> +			dev_deactivate(dev, !need_skip);
>  
>  		qdisc_offload_graft_root(dev, new, old, extack);
>  
> -		if (new && new->ops->attach && !ingress)
> +		if (need_skip)
>  			goto skip;
>  
>  		if (!ingress) {