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 <[email protected]>
---
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
Hi,
Vladimir Oltean <[email protected]> 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 <[email protected]>
> ---
The patch looks good:
Acked-by: Vinicius Costa Gomes <[email protected]>
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
On Tue, May 30, 2023 at 02:14:17PM -0700, Vinicius Costa Gomes wrote:
> 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.
Got it, you want to consolidate the dev_queue->qdisc_sleeping pattern.
Since taprio_dump_class() could benefit from the consolidation too, they
could really be both converted separately. Or I could also handle that
in this patch set, if I need to resend it.
Vladimir Oltean <[email protected]> writes:
> On Tue, May 30, 2023 at 02:14:17PM -0700, Vinicius Costa Gomes wrote:
>> 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.
>
> Got it, you want to consolidate the dev_queue->qdisc_sleeping pattern.
> Since taprio_dump_class() could benefit from the consolidation too, they
> could really be both converted separately. Or I could also handle that
> in this patch set, if I need to resend it.
Exactly. Both options sound great.
Cheers,
--
Vinicius