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

[Adriano Córdova]

El lun, 28 sept 2026 a las 11:38, Eric Dumazet (<edumazet@xxxxxxxxxx>) escribió:
>
> 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

Yes.

So there is not much to do about the TODO. I could send a v2 where
qdisc_change_tx_queue_len returns void and emits a warning on failure.