Message-ID: <CAM0EoM=P9+wNnNQ=ky96rwCx1z20fR21EWEdx+Na39NCqqG=3A@mail.gmail.com>
Date: Thu, 8 Jun 2023 14:44:46 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>, 
	Vinicius Costa Gomes <vinicius.gomes@...el.com>, linux-kernel@...r.kernel.org, 
	intel-wired-lan@...ts.osuosl.org, 
	Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>, Peilin Ye <yepeilin.cs@...il.com>, 
	Pedro Tammela <pctammela@...atatu.com>
Subject: Re: [PATCH RESEND net-next 5/5] net/sched: taprio: dump class stats
 for the actual q->qdiscs[]

On Fri, Jun 2, 2023 at 6:38 AM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> This makes a difference for the software scheduling mode, where
> dev_queue->qdisc_sleeping is the same as the taprio root Qdisc itself,
> but when we're talking about what Qdisc and stats get reported for a
> traffic class, the root taprio isn't what comes to mind, but q->qdiscs[]
> is.
>
> To understand the difference, I've attempted to send 100 packets in
> software mode through traffic class 0 (they are in the Qdisc's backlog),
> and recorded the stats before and after the change.
>

Other than the refcount issue i think the approach looks reasonable to
me. The stats before/after you are showing below though are
interesting; are you showing a transient phase where packets are
temporarily in the backlog. Typically the backlog is a transient phase
which lasts a very short period. Maybe it works differently for
taprio? I took a quick look at the code and do see to decrement the
backlog in the dequeue, so if it is not transient then some code path
is not being hit.

Aside: I realize you are busy - but if you get time and provide some
sample tc command lines for testing we could help create the tests for
you, at least the first time. The advantage of putting these tests in
tools/testing/selftests/tc-testing/ is that there are test tools out
there that run these tests and so regressions are easier to catch
sooner.

cheers,
jamal

> Here is before:
>
> $ tc -s class show dev eth0
> class taprio 8001:1 root leaf 8001:
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 9400b 100p requeues 0
>  Window drops: 0
> class taprio 8001:2 root leaf 8001:
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 9400b 100p requeues 0
>  Window drops: 0
> class taprio 8001:3 root leaf 8001:
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 9400b 100p requeues 0
>  Window drops: 0
> class taprio 8001:4 root leaf 8001:
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 9400b 100p requeues 0
>  Window drops: 0
> class taprio 8001:5 root leaf 8001:
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 9400b 100p requeues 0
>  Window drops: 0
> class taprio 8001:6 root leaf 8001:
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 9400b 100p requeues 0
>  Window drops: 0
> class taprio 8001:7 root leaf 8001:
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 9400b 100p requeues 0
>  Window drops: 0
> class taprio 8001:8 root leaf 8001:
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 9400b 100p requeues 0
>  Window drops: 0
>
> and here is after:
>
> class taprio 8001:1 root
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 9400b 100p requeues 0
>  Window drops: 0
> class taprio 8001:2 root
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 0b 0p requeues 0
>  Window drops: 0
> class taprio 8001:3 root
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 0b 0p requeues 0
>  Window drops: 0
> class taprio 8001:4 root
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 0b 0p requeues 0
>  Window drops: 0
> class taprio 8001:5 root
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 0b 0p requeues 0
>  Window drops: 0
> class taprio 8001:6 root
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 0b 0p requeues 0
>  Window drops: 0
> class taprio 8001:7 root leaf 8010:
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 0b 0p requeues 0
>  Window drops: 0
> class taprio 8001:8 root
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>  backlog 0b 0p requeues 0
>  Window drops: 0
>
> The most glaring (and expected) difference is that before, all class
> stats reported the global stats, whereas now, they really report just
> the counters for that traffic class.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  net/sched/sch_taprio.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index cc7ff98e5e86..23b98c3af8b2 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -2452,11 +2452,11 @@ static unsigned long taprio_find(struct Qdisc *sch, u32 classid)
>  static int taprio_dump_class(struct Qdisc *sch, unsigned long cl,
>                              struct sk_buff *skb, struct tcmsg *tcm)
>  {
> -       struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
> +       struct Qdisc *child = taprio_leaf(sch, cl);
>
>         tcm->tcm_parent = TC_H_ROOT;
>         tcm->tcm_handle |= TC_H_MIN(cl);
> -       tcm->tcm_info = dev_queue->qdisc_sleeping->handle;
> +       tcm->tcm_info = child->handle;
>
>         return 0;
>  }
> @@ -2466,8 +2466,7 @@ static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
>         __releases(d->lock)
>         __acquires(d->lock)
>  {
> -       struct netdev_queue *dev_queue = taprio_queue_get(sch, cl);
> -       struct Qdisc *child = dev_queue->qdisc_sleeping;
> +       struct Qdisc *child = taprio_leaf(sch, cl);
>         struct tc_taprio_qopt_offload offload = {
>                 .cmd = TAPRIO_CMD_TC_STATS,
>                 .tc_stats = {
> --
> 2.34.1
>

Powered by blists - more mailing lists