Re: [PATCH net-next 1/5] net/sched: taprio: don't overwrite "sch" variable in taprio_dump_class_stats()

Vinicius Costa Gomes
2023-05-30T21:15:15Z

Hi,

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> In taprio_dump_class_stats() we don't need a reference to the root Qdisc
> once we get the reference to the child corresponding to this traffic
> class, so it's okay to overwrite "sch". But in a future patch we will
> need the root Qdisc too, so create a dedicated "child" pointer variable
> to hold the child reference. This also makes the code adhere to a more
> conventional coding style.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

The patch looks good:

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

But I have a suggestion, this "taprio_queue_get() ->
dev_queue->qdisc_sleeping()" dance should have the same result as
calling 'taprio_leaf()'.

I am thinking of using taprio_leaf() here and in taprio_dump_class().
Could be a separate commit.

> net/sched/sch_taprio.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 76db9a10ef50..d29e6785854d 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -2388,10 +2388,10 @@ static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
> __acquires(d->lock)
> {
> struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
> + struct Qdisc *child = dev_queue->qdisc_sleeping;
>
> - sch = dev_queue->qdisc_sleeping;
> - if (gnet_stats_copy_basic(d, NULL, &sch->bstats, true) < 0 ||
> - qdisc_qstats_copy(d, sch) < 0)
> + if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 ||
> + qdisc_qstats_copy(d, child) < 0)
> return -1;
> return 0;
> }
> --
> 2.34.1
>

--
Vinicius