All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Hemminger <stephen@networkplumber.org>
To: "Hemendra M. Naik" <hemendranaik@gmail.com>
Cc: netdev@vger.kernel.org, jiri@resnulli.us, jhs@mojatatu.com,
	linux-kernel@vger.kernel.org, vishy0777@gmail.com,
	tahiliani@nitk.edu.in
Subject: Re: [PATCH iproute2-next v2] tc: fq_pie: add support for printing per-flow PIE statistics
Date: Sun, 14 Jun 2026 08:43:12 -0700	[thread overview]
Message-ID: <20260614084312.2bbcff11@phoenix.local> (raw)
In-Reply-To: <20260614130729.10076-1-hemendranaik@gmail.com>

On Sun, 14 Jun 2026 18:37:29 +0530
"Hemendra M. Naik" <hemendranaik@gmail.com> wrote:

> 'tc -s class show' against an fq_pie qdisc now prints:
> 
>  prob           drop probability for the flow
>  delay          per-flow queue sojourn time (microseconds)
>  deficit        remaining DRR byte credits (signed integer)
>  avg_dq_rate    dequeue rate estimate in bytes/second
>              	(dq_rate_estimator mode only)
> 
> avg_dq_rate is formatted using tc_print_rate(), which converts the
> kernel's bytes/second value to a human-readable bits/second string
> (e.g. '3906Kbit'), consistent with how other tc schedulers display
> rate fields. Apply the same fix to tc/q_pie.c, where avg_dq_rate was
> also printed as a raw integer without a unit.
> 
> Update the UAPI header to mirror tc_fq_pie_cl_stats from the kernel.
> Fix the 'delay' field comment in struct tc_pie_xstats from "in ms" to
> "in microseconds" to match the kernel's
> PSCHED_TICKS2NS / NSEC_PER_USEC conversion.
> 
> Add a 'tc -s class show' example to tc-fq_pie(8) with dq_rate_estimator
> enabled, showing all per-flow fields (prob, delay, deficit, avg_dq_rate)
> across multiple flows. Update tc-pie(8) avg_dq_rate example from a raw
> integer to a formatted bits/second string.
> 
> The corresponding kernel patch can be viewed here:
> https://lore.kernel.org/netdev/20260614125000.6058-1-hemendranaik@gmail.com/
> 
> Signed-off-by: Hemendra M. Naik <hemendranaik@gmail.com>
> Signed-off-by: Vishal Kamath <vishy0777@gmail.com>
> Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>

Minor feedback from AI review was:
Subject: Re: [PATCH iproute2-next v2] tc: fq_pie: add support for printing per-flow PIE statistics

On Sun, 14 Jun 2026, Hemendra M. Naik wrote:
> diff --git a/tc/q_fq_pie.c b/tc/q_fq_pie.c
> @@ -283,25 +285,43 @@ static int fq_pie_print_xstats(const struct qdisc_util *qu, FILE *f,
> +	if (st->type == TCA_FQ_PIE_XSTATS_CLASS) {
> +		print_float(PRINT_ANY, "prob", " prob %lg",
> +			    (double)st->class_stats.prob / (double)UINT64_MAX);
> +		print_uint(PRINT_JSON, "delay", NULL, st->class_stats.delay);
> +		print_string(PRINT_FP, NULL, " delay %s",
> +			     sprint_time(st->class_stats.delay, b1));
> +		print_int(PRINT_ANY, "deficit", " deficit %d",
> +			  st->class_stats.deficit);
> +
> +		if (st->class_stats.dq_rate_estimating) {
> +			tc_print_rate(PRINT_ANY, "avg_dq_rate", " avg_dq_rate %s",
> +				      st->class_stats.avg_dq_rate);
> +		}
> +	}
>  	print_nl();

The print_nl() at line 334 appears to be misplaced. It's outside both
conditional blocks, which means it will always print a newline regardless
of the statistics type being displayed. This could cause formatting issues:

- For TCA_FQ_PIE_XSTATS_CLASS, you'll get a newline after the class stats
- For qdisc stats, you'll get an extra newline after the memory_used field

The original code had print_nl() after the qdisc statistics. With the new
class statistics block, you likely need print_nl() inside each conditional
block to maintain proper formatting for each type.

Consider restructuring like this:
	if (!st->type || st->type == TCA_FQ_PIE_XSTATS_QDISC) {
		/* qdisc stats */
		...
		print_uint(PRINT_ANY, "memory_used", " memory_used %u",
			   st->memory_usage);
		print_nl();
	}

	if (st->type == TCA_FQ_PIE_XSTATS_CLASS) {
		/* class stats */
		...
		print_nl();
	}

Otherwise the patch looks good:
- Good use of print_* helpers throughout
- Proper handling of JSON vs text output modes
- The tc_print_rate() usage is correct and consistent
- Documentation updates in man pages are helpful


      reply	other threads:[~2026-06-14 15:43 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-14 13:07 [PATCH iproute2-next v2] tc: fq_pie: add support for printing per-flow PIE statistics Hemendra M. Naik
2026-06-14 15:43 ` Stephen Hemminger [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260614084312.2bbcff11@phoenix.local \
    --to=stephen@networkplumber.org \
    --cc=hemendranaik@gmail.com \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=tahiliani@nitk.edu.in \
    --cc=vishy0777@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.