* Re: [PATCH] Add --stats option to display event statistics
2025-09-18 10:59 [PATCH] Add --stats option to display event statistics Laurent Vivier
@ 2025-09-19 11:10 ` Stefano Brivio
0 siblings, 0 replies; 2+ messages in thread
From: Stefano Brivio @ 2025-09-19 11:10 UTC (permalink / raw)
To: Laurent Vivier; +Cc: passt-dev
On Thu, 18 Sep 2025 12:59:10 +0200
Laurent Vivier <lvivier@redhat.com> wrote:
> Introduce a new --stats DELAY option that displays event statistics
> tables showing counts by epoll event type. Statistics are printed to
> stderr with a minimum delay between updates, and only when events occur.
I guess we'll need more and more of these quirks in the future, so
maybe *at some point* we should think of optionally exporting those to
a machine-readable format.
Given that we'll probably need a small binary tool to dynamically
configure port forwarding, and address/port translations, maybe we
could, that day, have this also handled by... passtctl?
But meanwhile:
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> conf.c | 6 ++++++
> passt.1 | 6 ++++++
> passt.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> passt.h | 1 +
> 4 files changed, 77 insertions(+)
>
> diff --git a/conf.c b/conf.c
> index 02e903b5cf70..9aba27cef806 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -835,6 +835,8 @@ static void usage(const char *name, FILE *f, int status)
> "\n"
> " -d, --debug Be verbose\n"
> " --trace Be extra verbose, implies --debug\n"
> + " --stats DELAY Display events statistics\n"
> + " minimum DELAY seconds between updates\n"
> " -q, --quiet Don't print informational messages\n"
> " -f, --foreground Don't run in background\n"
> " default: run in background\n"
> @@ -1480,6 +1482,7 @@ void conf(struct ctx *c, int argc, char **argv)
> {"repair-path", required_argument, NULL, 28 },
> {"migrate-exit", no_argument, NULL, 29 },
> {"migrate-no-linger", no_argument, NULL, 30 },
> + {"stats", required_argument, NULL, 31 },
> { 0 },
> };
> const char *optstring = "+dqfel:hs:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:";
> @@ -1708,6 +1711,9 @@ void conf(struct ctx *c, int argc, char **argv)
> die("--migrate-no-linger is for vhost-user mode only");
> c->migrate_no_linger = true;
>
> + break;
> + case 31:
> + c->stats = strtol(optarg, NULL, 0);
We should make sure that we print to standard error only if it's there,
and not to, say, a TCP socket that happened to be number 2. See also
commit b74801645c23 ("conf, passt: Don't try to log to stderr after we
close it") for how badly I broke this in the past.
We could probably go the same way as that commit for c->stats as well.
> break;
> case 'd':
> c->debug = 1;
> diff --git a/passt.1 b/passt.1
> index af5726a7f1c5..d5cde2c424d0 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -84,6 +84,12 @@ Be verbose, don't log to the system logger.
> .BR \-\-trace
> Be extra verbose, show single packets. Implies \fB--debug\fR.
>
> +.TP
> +.BR \-\-stats " " \fIDELAY\fR
> +Display events statistics with a
> +minimum \fIDELAY\fR seconds between updates. If there is no event,
> +statistics are not displayed.
Unnecessary padding/wrapping I guess? This could be two lines.
> +
> .TP
> .BR \-q ", " \-\-quiet
> Don't print informational messages.
> diff --git a/passt.c b/passt.c
> index a4ec115d1784..afd7f709e58a 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -83,6 +83,31 @@ char *epoll_type_str[] = {
> static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES,
> "epoll_type_str[] doesn't match enum epoll_type");
>
> +char *epoll_type_tag[] = {
This could be const, and I'm a bit surprised none of the static
checkers suggests that, and that's the same for epoll_type_str[],
actually.
But I find those names a bit hard to decode, so, instead of having this
array, what about my proposal below? See comment to print_stats().
> + [EPOLL_TYPE_TCP] = "TCP",
> + [EPOLL_TYPE_TCP_SPLICE] = "TCPSPL",
> + [EPOLL_TYPE_TCP_LISTEN] = "TCPLSN",
> + [EPOLL_TYPE_TCP_TIMER] = "TCPTMR",
> + [EPOLL_TYPE_UDP_LISTEN] = "UDPLSN",
> + [EPOLL_TYPE_UDP] = "UDP",
> + [EPOLL_TYPE_PING] = "PING",
> + [EPOLL_TYPE_NSQUIT_INOTIFY] = "INOTF",
> + [EPOLL_TYPE_NSQUIT_TIMER] = "TIMER",
> + [EPOLL_TYPE_TAP_PASTA] = "PASTA",
> + [EPOLL_TYPE_TAP_PASST] = "PASST",
> + [EPOLL_TYPE_TAP_LISTEN] = "TAPLSN",
> + [EPOLL_TYPE_VHOST_CMD] = "VUCMD",
> + [EPOLL_TYPE_VHOST_KICK] = "VUKICK",
> + [EPOLL_TYPE_REPAIR_LISTEN] = "REPLSN",
> + [EPOLL_TYPE_REPAIR] = "REPAIR",
> +};
> +static_assert(ARRAY_SIZE(epoll_type_tag) == EPOLL_NUM_TYPES,
> + "epoll_type_tag[] doesn't match enum epoll_type");
> +
This should have the usual kerneldoc-style description.
> +struct passt_stats {
> + unsigned long events[EPOLL_NUM_TYPES];
> +};
> +
> /**
> * post_handler() - Run periodic and deferred tasks for L4 protocol handlers
> * @c: Execution context
> @@ -174,6 +199,42 @@ static void exit_handler(int signal)
> _exit(EXIT_SUCCESS);
> }
>
> +/**
> + * print_stats() - Print event statistics table to stderr
> + * @c: Execution context
> + * @stats: Event counters
> + * @now: Current timestamp
> + */
> +static void print_stats(const struct ctx *c, const struct passt_stats *stats,
> + const struct timespec *now)
> +{
> + int i;
> + static struct timespec before;
> + long long elapsed_ns;
> +
> + if (!c->stats)
> + return;
> +
> + elapsed_ns = (now->tv_sec - before.tv_sec) * 1000000000LL +
> + (now->tv_nsec - before.tv_nsec);
> +
> + if (elapsed_ns < c->stats * 1000000000LL)
> + return;
> +
> + before = *now;
> +
> + /* table header */
> + FPRINTF(stderr, "\t");
> + for (i = 1; i < EPOLL_NUM_TYPES; i++)
> + FPRINTF(stderr, "%6s\t", epoll_type_tag[i]);
Instead of being forced to mangle header fields to fit, what about
having a multi-line header that's only printed every now and then, say,
every slightly less than the canonical 22 rows? That is (quickly
tested):
---
...
static int lines_printed;
long long elapsed_ns;
if (!c->stats)
return;
elapsed_ns = (now->tv_sec - before.tv_sec) * 1E9 +
(now->tv_nsec - before.tv_nsec);
if (elapsed_ns < c->stats * 1E9)
return;
before = *now;
if (!(lines_printed % 20)) {
/* Table header */
for (i = 1; i < EPOLL_NUM_TYPES; i++) {
int j;
for (j = 0; j < i * (6 + 1); j++) {
if (j && !(j % (6 + 1)))
FPRINTF(stderr, "|");
else
FPRINTF(stderr, " ");
}
FPRINTF(stderr, "%s\n", epoll_type_str[i]);
}
}
FPRINTF(stderr, " ");
for (i = 1; i < EPOLL_NUM_TYPES; i++)
FPRINTF(stderr, " %6lu", stats->events[i]);
FPRINTF(stderr, "\n");
lines_printed++;
...
---
?
> + FPRINTF(stderr, "\n");
> + /* main loop */
> + FPRINTF(stderr, "MAIN\t");
> + for (i = 1; i < EPOLL_NUM_TYPES; i++)
> + FPRINTF(stderr, "%6lu\t", stats->events[i]);
> + FPRINTF(stderr, "\n");
> +}
> +
> /**
> * main() - Entry point and main loop
> * @argc: Argument count
> @@ -196,6 +257,7 @@ int main(int argc, char **argv)
> struct rlimit limit;
> struct timespec now;
> struct sigaction sa;
> + struct passt_stats stats = { 0 };
This should go a bit above.
>
> if (clock_gettime(CLOCK_MONOTONIC, &log_start))
> die_perror("Failed to get CLOCK_MONOTONIC time");
> @@ -362,6 +424,8 @@ loop:
> /* Can't happen */
> ASSERT(0);
> }
> + stats.events[ref.type]++;
> + print_stats(&c, &stats, &now);
> }
>
> post_handler(&c, &now);
> diff --git a/passt.h b/passt.h
> index 3ffc19fdae05..283e7c8a832c 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -248,6 +248,7 @@ struct ctx {
> enum passt_modes mode;
> int debug;
> int trace;
> + int stats;
And this should be documented in the struct comment.
> int quiet;
> int foreground;
> int nofile;
The rest looks good and quite useful to me (I guess it's much more
useful to you, of course ;)).
--
Stefano
^ permalink raw reply [flat|nested] 2+ messages in thread