diff mbox series

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

Message ID 20230530091948.1408477-2-vladimir.oltean@nxp.com
State New
Headers
Series xstats for tc-taprio |

Commit Message

Vladimir Oltean May 30, 2023, 9:19 a.m. UTC 
  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>
---
 net/sched/sch_taprio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Vinicius Costa Gomes May 30, 2023, 9:14 p.m. UTC | #1 
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
>
Vladimir Oltean May 30, 2023, 9:32 p.m. UTC | #2 
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.
Vinicius Costa Gomes May 30, 2023, 10:33 p.m. UTC | #3 
Vladimir Oltean <vladimir.oltean@nxp.com> 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,
diff mbox series

Patch

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;
 }