Re: [PATCH net-next] net: sched: revert tx_queue_len on partial qdisc resize failure

[Eric Dumazet]

On Mon, Sep 28, 2026 at 4:22 PM Adriano Cordova <adrianox@xxxxxxxxx> wrote:
>
> Pass the old length to dev_qdisc_change_tx_queue_len() and, when a queue
> fails to resize, use it to restore the queues already handled.
>
> Fixes: 48bfd55e7e41 ("net_sched: plug in qdisc ops change_tx_queue_len")
> Signed-off-by: Adriano Cordova <adrianox@xxxxxxxxx>
> ---
>  include/net/sch_generic.h |  2 +-
>  net/core/dev.c            |  2 +-
>  net/sched/sch_generic.c   | 15 +++++++++------
>  3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index f35bd06a6bad..0ce97b4b2da1 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -720,7 +720,7 @@ void qdisc_class_hash_remove(struct Qdisc_class_hash *,
>  void qdisc_class_hash_grow(struct Qdisc *, struct Qdisc_class_hash *);
>  void qdisc_class_hash_destroy(struct Qdisc_class_hash *);
>
> -int dev_qdisc_change_tx_queue_len(struct net_device *dev);
> +int dev_qdisc_change_tx_queue_len(struct net_device *dev, unsigned int old_len);
>  void dev_qdisc_change_real_num_tx(struct net_device *dev,
>                                   unsigned int new_real_tx);
>  void dev_init_scheduler(struct net_device *dev);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index f660fccfc0db..57e1fa320d53 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10002,7 +10002,7 @@ int netif_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
>                 res = notifier_to_errno(res);
>                 if (res)
>                         goto err_rollback;
> -               res = dev_qdisc_change_tx_queue_len(dev);
> +               res = dev_qdisc_change_tx_queue_len(dev, orig_len);
>                 if (res)
>                         goto err_rollback;
>         }
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 6f6a6f0d5eb0..8fd0fce23cc4 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1422,13 +1422,14 @@ void dev_deactivate(struct net_device *dev, bool reset_needed)
>  EXPORT_SYMBOL(dev_deactivate);
>
>  static int qdisc_change_tx_queue_len(struct net_device *dev,
> -                                    struct netdev_queue *dev_queue)
> +                                    struct netdev_queue *dev_queue,
> +                                    unsigned int len)
>  {
>         struct Qdisc *qdisc = rtnl_dereference(dev_queue->qdisc_sleeping);
>         const struct Qdisc_ops *ops = qdisc->ops;
>
>         if (ops->change_tx_queue_len)
> -               return ops->change_tx_queue_len(qdisc, dev->tx_queue_len);
> +               return ops->change_tx_queue_len(qdisc, len);
>         return 0;
>  }
>
> @@ -1465,7 +1466,7 @@ void mq_change_real_num_tx(struct Qdisc *sch, unsigned int new_real_tx)
>  }
>  EXPORT_SYMBOL(mq_change_real_num_tx);
>
> -int dev_qdisc_change_tx_queue_len(struct net_device *dev)
> +int dev_qdisc_change_tx_queue_len(struct net_device *dev, unsigned int old_len)
>  {
>         bool up = dev->flags & IFF_UP;
>         unsigned int i;
> @@ -1475,13 +1476,15 @@ int dev_qdisc_change_tx_queue_len(struct net_device *dev)
>                 dev_deactivate(dev, false);
>
>         for (i = 0; i < dev->num_tx_queues; i++) {
> -               ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i]);
> -
> -               /* TODO: revert changes on a partial failure */
> +               ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i],
> +                                               dev->tx_queue_len);
>                 if (ret)
>                         break;
>         }
>
> +       while (ret && i--)
> +               qdisc_change_tx_queue_len(dev, &dev->_tx[i], old_len);
> +

Ah... Probably driven by the TODO  comment and a LLM ?

One caveat is that pfifo_fast_change_tx_queue_len() is currently the
only ->change_tx_queue_len() implementation, and it only fails on
-ENOMEM (inside skb_array_resize_multiple_bh() after freeing the
previous ring arrays on already-processed queues).

Trying to revert queues 0..i-1 back to old_len will therefore have to
allocate new ptr_ring arrays right after an -ENOMEM failure (which is
especially likely to fail again if old_len > new_len), and cannot
recover packets already dropped if the ring was shrunk.

pw-bot: cr