hello Jakub, and thanks for checking!
On Thu, 2020-12-17 at 11:05 -0800, Jakub Kicinski wrote:
> On Wed, 16 Dec 2020 19:33:29 +0100 Davide Caratti wrote:
> > + if (q->qdiscs) {
> > + for (i = 0; i < dev->num_tx_queues && q->qdiscs[i]; i++)
> > + qdisc_reset(q->qdiscs[i]);
>
> Are you sure that we can't graft a NULL in the middle of the array?
that should not happen, because child qdiscs are checked for being non-
NULL when they are created:
https://elixir.bootlin.com/linux/v5.10/source/net/sched/sch_taprio.c#L1674
and then assigned to q->qdiscs[i]. So, there might be NULL elements of
q->qdiscs[] in the middle of the array when taprio_reset() is called,
but it should be ok to finish the loop when we encounter the first one:
subsequent ones should be NULL as well.
> Shouldn't this be:
>
> for (i = 0; i < dev->num_tx_queues; i++)
> if (q->qdiscs[i])
> qdisc_reset(q->qdiscs[i]);
>
> ?
probably the above code is more robust, but - like you noticed - then we
should also change the same 'for' loop in taprio_destroy(), otherwise it
leaks resources. If you and Vinicius agree, I can post a follow-up patch
that makes ->reset() and ->destroy() more consistent with ->enqueue()
and ->dequeue(), and send it for net-next when it reopens. Ok?
--
davide
